-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Dataflow: Replace stage duplication with parameterised modules. #9823
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
The removal of that error message is currently still blocked by https://github.com/github/semmle-code/pull/42236. Once we have the frontend binding set checker, we can remove the warning and proceed with this. |
04aa5df
to
259561c
Compare
The blocking warning should be gone, now that we merged https://github.com/github/semmle-code/pull/43179 into main. |
259561c
to
a5260b1
Compare
d1a7794
to
3d47875
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Please do a DCA run for all languages as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could add a // TODO: member predicate on `Ap`
comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could add a // TODO: member predicate on `Ap`
comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have two calls to MkStage<Stage1>
(and same for other stages), here and in the module Stage2
. Perhaps that is OK, as parameterized modules are not generative?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is indeed perfectly fine.
DCA looks good, I think. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💪
Agreed for C++. The vim failure is expected (we're working on it). |
Merge commit: aa36556
Merge commit: aa36556
Merge commit: aa36556
Merge commit: aa36556
Appears to work, although there are some complaints of the form
@ginsbach