C++: match extractor changes to two-operand ?: representation #3879
Conversation
nickrolfe
added
C++
depends on internal PR
|
I think both the consistency query results and the simple range analysis changes will need library fixes. |
|
I think there is an extractor issue for cases where there isn't an implicit cast on the 'then' expr. I'm investigating. |
adds another query that showed a bug in the extractor
|
That extractor issue has been fixed, but the consistency query and simple range analysis results haven't changed. I'm not sure what kind of library changes are required there. |
| # 1308| expr: [ParenthesisExpr] (...) | ||
| # 1308| Type = [BoolType] bool | ||
| # 1308| ValueCategory = prvalue | ||
| # 1308| expr: [LogicalOrExpr] ... || ... |
There was a problem hiding this comment.
Is this the exact same expression object as child 0, or is it a copy? What happens if operand 0 might have side effects, as in foo() ?: 7?
There was a problem hiding this comment.
In this case, this is a separate entity in the db, but getting the same entity is possible, such as in your foo() example.
You can see an example of an expr being reused like that in question_mark_colon/types.expected that I added in this PR.
|
@jbj Are you OK with us merging this PR (and the extractor change) now, with an issue opened for me to go back and fix the IR consistency failures? Or should I try to contribute the necessary IR construction fixes to this PR before we merge it? |
|
I made this change in response to the extractor issue you raised, so if it's not a net-positive for IR consistency, I have no issue with postponing the merge. |
|
I believe (but have not confirmed) that your change to fix the extractor issue merely breaks the workaround that I was using before. I just have to go back into the IR construction code and change it do do the right thing, rather than the workaround. |
|
Ping @dbartol. |
Closes https://github.com/github/codeql-c-extractor-team/issues/67
Now the extractor emits a synthesized expr node for the 'then' case.
I've updated the
question_mark_colontest to show that the 'then' expr now includes any implicit cast.The upgrade script populates the missing
expr_cond_truetuples by re-using the guard expr (just like the old implementation ofgetThen()).I could use some assistance in judging whether the new results for the consistency queries are a problem.