Skip to content

Conversation

@maximiliankaul
Copy link
Contributor

Currently, when creating nodes in an EOGConceptPass they are first added to a set and then later created via some pass specific internal mechanisms. This means that the order when creating multiple nodes in the pass and the order when they are actually created internally might differ, resulting in unexpected EOG results for example.

  • Do we need to change more sets to ordered sets?
  • Do we want to have this change?

@codecov
Copy link

codecov bot commented May 28, 2025

Codecov Report

Attention: Patch coverage is 20.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 78.24%. Comparing base (da6424c) to head (b3ec4f8).

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...ec/cpg/helpers/functional/BasicLatticesRedesign.kt 0.00% 4 Missing ⚠️

❌ Your patch check has failed because the patch coverage (20.00%) is below the target coverage (75.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
Files with missing lines Coverage Δ
.../fraunhofer/aisec/cpg/passes/UnreachableEOGPass.kt 88.03% <100.00%> (ø)
...ec/cpg/helpers/functional/BasicLatticesRedesign.kt 90.86% <0.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Member

@oxisto oxisto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This definitely needs a test case to find out if it's really working as intended. Generally I am not a fan of specifying a specific implementation (LinkedHashSet) where we previously had a Set interface, but I guess there is no OrderedSet interface or similar.

@KuechA
Copy link
Contributor

KuechA commented May 28, 2025

Actually, I'm pretty sure that these changes do not have the intended effect. The PR changes the order in which a Lattice would keep its elements. However, based on the description of the PR, I think that the intention is to change the implementation of PowersetLattice.Element (which currently inherits from IdentitySet) to a LinkedHashSet. That's where we keep the OverlayNodes which are created by the EOGConceptPasses.

override lateinit var elements: Set<Element<T>>
override lateinit var elements: LinkedHashSet<Element<T>>

class Element<T>(expectedMaxSize: Int) : IdentitySet<T>(expectedMaxSize), Lattice.Element {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably you want to change this line of code. We need a very fast and scalable implementation of a Set which keeps the order and compares based on the identity, not equality.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants