Fix dangling-else bugs in FAIL_FAST_IF and EMIT_USER_WARNING macros - #41504
Conversation
Both macros were bare if-statements without do/while(0) guards, causing the dangling else problem when used as a single statement under an if.
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
🟢 Approval recommended
The macro-guard changes correctly address the dangling-else control-flow bug, and remaining feedback is limited to non-blocking formatting consistency.
Pull request overview
This PR fixes a classic “dangling else” macro hazard in the Linux-side WIL shim by ensuring multi-statement macros expand as a single statement, preventing else from binding to an internal if.
Changes:
- Wrapped
FAIL_FAST_IFin ado { } while (0)-style guard. - Wrapped
EMIT_USER_WARNINGin ado { } while (0)-style guard.
File summaries
| File | Description |
|---|---|
| src/linux/inc/lxwil.h | Makes FAIL_FAST_IF and EMIT_USER_WARNING safe in if/else single-statement contexts by adding a statement-guard wrapper. |
Review details
Suppressed comments (1)
src/linux/inc/lxwil.h:658
- For consistency with other guarded multi-statement macros in this file (e.g., WI_SetFlagIf), prefer placing the opening brace for the do/while guard on its own line.
#define EMIT_USER_WARNING(Warning) \
do { \
if (::wil::ScopedWarningsCollector::CanCollectWarning()) \
{ \
::wil::ScopedWarningsCollector::CollectWarning(Warning); \
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟢 Approval recommended
The functional fix matches the PR description and addresses the dangling-else hazard; remaining feedback is limited to minor formatting consistency within macro bodies.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
🟡 Changes recommended
The Windows EMIT_USER_WARNING definition still has the same dangling-else vulnerability.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Balanced
| { \ | ||
| ::wil::ScopedWarningsCollector::CollectWarning(Warning); \ | ||
| } | ||
| do { \ |
There was a problem hiding this comment.
🔵 Needs a closer look
Formatting violations remain, and the Windows macro still has the dangling-else bug.
Review details
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
src/linux/inc/lxwil.h:655
- This second
dostatement also violates the repository's Allman brace convention;WI_SetFlagIfatsrc/linux/inc/lxwil.h:639-646shows the established macro formatting. Put the opening brace on its own continued line.
src/linux/inc/lxwil.h:654
- The Windows definition at
src/windows/common/ExecutionContext.h:37-41is still an unguardedif, soEMIT_USER_WARNINGretains the same dangling-elsebug for Windows callers. Apply the equivalent single-statement guard to that definition as part of this fix.
#define EMIT_USER_WARNING(Warning) \
src/linux/inc/lxwil.h:19
- This control statement does not follow the repository's Allman brace convention; the nearby
WI_SetFlagIfmacro atsrc/linux/inc/lxwil.h:639-646puts the opening brace on the next continued line. Splitdoand{to keep this macro consistent with the file's established formatting.
do { \
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Ben Hillis (benhillis)
left a comment
There was a problem hiding this comment.
Looks good, thank you for the contribution.
Summary of the Pull Request
FAIL_FAST_IFandEMIT_USER_WARNINGwere missing the standarddo { } while(0)macro guard. When used as the single statement branch of an enclosingif/else, theelsewould incorrectly bind to the macro's internalifinstead of the outerif, silently inverting control flow.Windows counterpart: #41513
PR Checklist
FAIL_FAST_IFandEMIT_USER_WARNINGDetailed Description of the Pull Request / Additional comments
Changes
FAIL_FAST_IFindo { } while ((void)0, 0)EMIT_USER_WARNINGindo { } while ((void)0, 0)Both macros were previously defined as bare
ifstatements, which left callers vulnerable to dangling-else misassociation when used without braces:The do { ... } while ((void)0, 0) wrapper ensures each macro always expands to a single statement, preventing the outer else from binding to the internal if.
Validation Steps Performed
another PR #41342