C++: Support if consteval and if ! consteval
#18502
Conversation
jketema
added
the
depends on internal PR
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
jketema
force-pushed
the
consteval
branch
2 times, most recently
from
c4673e1 to
f70bf5b
Compare
| p1.nodeAt(n1, s) and | ||
| p2.nodeBefore(n2, s.getThen()) | ||
| or | ||
| p1.nodeAt(n1, s) and | ||
| p2.nodeBefore(n2, s.getElse()) |
There was a problem hiding this comment.
Here we allow both branches of the consteval if to be reached.
| TranslatedStmt getEvaluatedBranch() { | ||
| result = getTranslatedStmt(stmt.getRuntimeEvaluatedBranch()) | ||
| } |
There was a problem hiding this comment.
We pick a single branch that can be reached. For now this is the branch that is taken at runtime, we can change this later if we see that using the compile time branch improves analysis quality.
cpp/downgrades/1aa71a4a687fc93f807d4dfeeef70feceeced242/stmts.ql
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I wasn't able to give the IR a full review.
Overall LGTM, feel free to ignore class naming suggestions.
I wasn't sure about the need for consteval_if_then/2 and consteval_if_else/2 tables in the dbscheme as this doesn't seem to align with other parent/child relationships.
| * } | ||
| * ``` | ||
| */ | ||
| class ConstevalOrNotConstevalIfStmt extends Stmt, @stmt_consteval_or_not_consteval_if { |
There was a problem hiding this comment.
Nit: I would personally be ok by calling this class ConstevalIfStmt and just having an isNot() predicate on it. Users might use ConstevalIfStmt and forget about its complement.
There was a problem hiding this comment.
Nit: Or even IfConstevalStmt and IfNotConsteval since that is the reading order.
There was a problem hiding this comment.
ConstevalIfStmt is in line with ConstexprIfStmt, which has as syntax if constexpr.
There was a problem hiding this comment.
Folded the classes and introduced the isNot.
| @@ -2194,6 +2196,18 @@ constexpr_if_else( | |||
| int else_id: @stmt ref | |||
| ); | |||
|
|
|||
| @stmt_consteval_or_not_consteval_if = @stmt_consteval_if | @stmt_not_consteval_if; | |||
|
|
|||
| consteval_if_then( | |||
There was a problem hiding this comment.
I think we should use getChild(1) and getChild(2) for this to be consistent withIfStmt, then we don't need these tables.
There was a problem hiding this comment.
What I'm doing here is consistent with if. See the if_then and if_else tables just above here.
This aligns with what is done for |
Oh I see! Yes, I see that all the If stmts have been written in a similar way. |
|
The IR construction changes LGTM! I'll let someone from the team approve the PR 🙂 |
Pull Request checklist
All query authors
.qhelp. See the documentation in this repository.Internal query authors only
.ql,.qll, or.qhelpfiles. See the documentation (internal access required).