Conversation
b994559 to
04cb9e4
Compare
There was a problem hiding this comment.
Pull Request Overview
Adds support for the Microsoft-specific __leave statement in the C++ QL libraries and tests.
- Introduces a new
LeaveStmtclass and maps it to a fresh statement kind. - Updates the semmlecode schema, upgrade/downgrade scripts, and change notes.
- Adds library tests (
leave.cpp,leave.ql,leave.expected) to verify detection and enclosing-try resolution.
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| cpp/ql/lib/semmlecode.cpp.dbscheme | Add 40 = @stmt_leave and extend @jump to include @stmt_leave |
| cpp/ql/lib/semmle/code/cpp/stmts/Stmt.qll | Define LeaveStmt class, implement getEnclosingTry recursive lookup |
| cpp/ql/lib/upgrades/3c45f8b9e71ec723bf50c40581e1f18f4f25e290/upgrade.properties | Declare upgrade metadata for __leave support |
| cpp/ql/lib/change-notes/2025-06-11-leave-stmt.md | Document feature addition of __leave statement |
| cpp/downgrades/a8c2176e9a5cf9be8d17053a4c8e7e56b5aced6d/upgrade.properties | Declare downgrade metadata for __leave support |
| cpp/downgrades/a8c2176e9a5cf9be8d17053a4c8e7e56b5aced6d/stmts.ql | Map legacy kinds, translating stmt_leave back to kind 4 |
| cpp/ql/test/library-tests/stmt/leave/leave.ql | Add QL test query selecting LeaveStmt and its enclosing try |
| cpp/ql/test/library-tests/stmt/leave/leave.expected | Add expected results for leave.ql query |
| cpp/ql/test/library-tests/stmt/leave/leave.cpp | Add C++ source with __leave in both __finally and __except contexts |
Comments suppressed due to low confidence (1)
cpp/ql/lib/upgrades/3c45f8b9e71ec723bf50c40581e1f18f4f25e290/upgrade.properties:2
- The upgrade properties do not include a
stmts.rel: run stmts.qlodirective, so upgrade tests might not exercise the new__leaveupgrade rules. Consider adding this to ensure the upgrade path is exercised.
compatibility: partial
| } | ||
|
|
||
| private MicrosoftTryStmt getEnclosingTry(Stmt s) { | ||
| if s.getParent().getEnclosingStmt() instanceof MicrosoftTryStmt |
There was a problem hiding this comment.
The recursive helper getEnclosingTry lacks a base case when no enclosing MicrosoftTryStmt exists, potentially leading to infinite recursion. Consider adding a guard that returns none() when reaching the root or a non-stmt parent.
| if s.getParent().getEnclosingStmt() instanceof MicrosoftTryStmt | |
| if s.getParent() = none() or s.getParent().getEnclosingStmt() = none() | |
| then result = none() | |
| else if s.getParent().getEnclosingStmt() instanceof MicrosoftTryStmt |
| | 37 = @stmt_co_return | ||
| | 38 = @stmt_consteval_if | ||
| | 39 = @stmt_not_consteval_if | ||
| | 40 = @stmt_leave |
There was a problem hiding this comment.
[nitpick] The new stmt_leave enum entry is added out of numeric order among existing cases, which can make future maintenance harder. Consider inserting it in sorted order or updating surrounding comments.
jketema
left a comment
There was a problem hiding this comment.
Two small comments, otherwise LGTM.
cpp/downgrades/a8c2176e9a5cf9be8d17053a4c8e7e56b5aced6d/upgrade.properties
Outdated
Show resolved
Hide resolved
674a02a to
7af8287
Compare
This PR adds support to the Microsoft-specific
__leavestatement.