Skip to content

Conversation

geoffw0
Copy link
Contributor

@geoffw0 geoffw0 commented Jan 9, 2019

@lukecartey has said that the MacroInvocationExpr class is confusing and commonly misunderstood by new [and if we're honest, not so new] QL authors. I tend to agree, and I further believe that when used correctly, it often just adds a layer of obfuscation over using MacroInvocation.getExpr() directly. I have deprecated it and replaced it's usage in our libraries.

MacroInvocationStmt was not used but suffers the same problems in principle. Also deprecated.

I've also added test cases for ArithmeticUncontrolled.ql. The use (isRandValue) in ArithmeticUncontrolled.ql is really quite dubious and should be replaced with cleaner logic that's able to handle the same cases at some point.

@geoffw0 geoffw0 added the C++ label Jan 9, 2019
@geoffw0 geoffw0 requested a review from a team as a code owner January 9, 2019 12:20
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.

LGTM, but the failing tests show that you need to update our internal repo before we can merge this. There are 8 files using MacroInvocationExpr in our internal repo according to git grep.

@geoffw0
Copy link
Contributor Author

geoffw0 commented Jan 23, 2019

I've created https://git.semmle.com/Semmle/code/pull/30211 to address the tests. Once that's merged I'll rebase this and/or add a submodule update, which should resolve the test failures.

@jbj
Copy link
Contributor

jbj commented Jan 24, 2019

I've merged the internal PR, and I think that should be enough to make Language-Tests/CPP pass without any further work. I'll re-trigger them.

@jbj jbj merged commit 2aca40a into github:master Jan 25, 2019
cklin pushed a commit that referenced this pull request May 20, 2022
Release preparation for version 2.9.2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants