Skip to content

Conversation

andersfugmann
Copy link
Contributor

Addressing review comment on #6568: This Pr uses the new predicate for RangeSSA::isGuardPhi/4.
I have not deleted the old predicate RangeSSA::isGuardPhi/3, even though its not used anymore to avoid braking of external queries which might rely on RangeSSA::isGuardPhi/3

@andersfugmann andersfugmann requested a review from a team as a code owner September 13, 2021 07:48
@github-actions github-actions bot added the C++ label Sep 13, 2021
@andersfugmann andersfugmann added the no-change-note-required This PR does not need a change note label Sep 13, 2021
@jbj
Copy link
Contributor

jbj commented Sep 13, 2021

I suggest marking RangeSSA::isGuardPhi/3 as deprecated. It seems there wasn't a single need for it in our own codebase, and we found out it can be a footgun. Then a change note is needed.

As these changes affect the join order, a DCA run would be appropriate.

@andersfugmann
Copy link
Contributor Author

I suggest marking RangeSSA::isGuardPhi/3 as deprecated. It seems there wasn't a single need for it in our own codebase, and we found out it can be a footgun. Then a change note is needed.

I will deprecate the function.

As these changes affect the join order, a DCA run would be appropriate.

DCA timings looks ok.

@jbj
Copy link
Contributor

jbj commented Sep 15, 2021

There's an unrelated integration-test failure:

Running TRAP import for CodeQL database at /home/runner/work/semmle-code/semmle-code/integration-tests/posix-only/multi_extract/cluster/javascript...
foo.js.trap.gz, 3: com.semmle.util.exception.CatastrophicError: Tried to insert tuple with 2 columns into files which has 5 columns.

Maybe merging from main will fix it.

Copy link
Contributor

@jbj jbj left a comment

Choose a reason for hiding this comment

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

Whoops, this PR had fallen off my radar. LGTM.

@jbj jbj merged commit 914e621 into github:main Sep 28, 2021
@andersfugmann andersfugmann deleted the refactor_use_of_isGuardPhi branch September 29, 2021 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C++ no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants