Skip to content

Conversation

jbj
Copy link
Contributor

@jbj jbj commented Apr 6, 2020

This workaround from DataFlowUtil.qll (#1781) should be useful for any query that selects an Expr. In particular, it's useful for IR data flow.

Internal PR: https://git.semmle.com/Semmle/code/pull/37020

To do:

  • Run CPP-Differences and use the Profile Viewer.
  • Accept test result changes in internal and external repo.

This workaround from `DataFlowUtil.qll` should be useful for any query
that selects an `Expr`. In particular, it's useful for IR data flow.

This commit does not include test changes.
@jbj jbj added the C++ label Apr 6, 2020
@jbj jbj requested a review from geoffw0 April 6, 2020 12:18
@jbj
Copy link
Contributor Author

jbj commented Apr 6, 2020

All the test changes in Language-Tests/CPP LGTM, and I think they just need to be accepted.

Because this PR conflicts with Absolutely Everything and will need a synced submodule update, I'm going to wait until a less busy time before proposing it for merge.

@jbj
Copy link
Contributor Author

jbj commented May 2, 2020

I'd like to get #3389 merged first, and then I can remove the getActualLocation predicate that's introduced (via rename) there.

Copy link
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

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

QL changes LGTM.

Because this PR conflicts with Absolutely Everything

I assume the conflicts will be limited to tests, and perhaps mostly IR/dataflow tests, so not quite as bad as you describe here?

@jbj
Copy link
Contributor Author

jbj commented May 7, 2020

I assume the conflicts will be limited to tests, and perhaps mostly IR/dataflow tests, so not quite as bad as you describe here?

I think this will include tests in the internal repo, so there will also be potential conflicts on the submodule pointer.

@jbj
Copy link
Contributor Author

jbj commented May 28, 2020

@jbj jbj added the depends on internal PR This PR should only be merged in sync with an internal Semmle PR label May 28, 2020
@jbj jbj marked this pull request as ready for review May 28, 2020 19:06
@jbj jbj requested a review from a team as a code owner May 28, 2020 19:06
jbj added 2 commits May 29, 2020 08:44
…karound

Conflicts and semantic conflicts in `library-tests/dataflow/fields` and
`library-tests/ir/ir`.
…karound

Conflicts:
	cpp/ql/test/library-tests/dataflow/fields/dataflow-ir-consistency.expected
@jbj
Copy link
Contributor Author

jbj commented May 29, 2020

I thought I'd picked a quiet time to get these conflict-prone changes merged, but apparently it was just the opposite. I can't complain about that.

The CPP-Differences results from yesterday should still be valid. Performance is unchanged, and there are two new results on a hidden-by-default query. Those results must have been there the whole time, just hidden because locations were missing.

@MathiasVP MathiasVP merged commit ae4f6ed into github:master May 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C++ depends on internal PR This PR should only be merged in sync with an internal Semmle PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants