gribozavr2 added inline comments.
================ Comment at: clang-tools-extra/clang-tidy/misc/AssignmentInIfClauseCheck.cpp:39 + diag(MatchedDecl->getBeginLoc(), + "Assignment detected within if statement. Fix to equality check if this " + "was accidental. Consider moving out of if statement if intentional."); ---------------- Please follow Clang's message style. The message should start with a lowercase letter and should not end with a period. Suggested fixes (since there are multiple) should be in notes. warning: an assignment within an 'if' condition is bug-prone note: if it should be an assignment, move it out of the 'if' condition note: if it is meant to be an equality check, change '=' to '==' Also consider adding a fixit to the second note. ================ Comment at: clang-tools-extra/docs/clang-tidy/checks/misc-assignment-in-if-clause.rst:3 + +misc-assignment-in-if-clause +============================ ---------------- WDYT about `bugprone-assignment-in-if-condition`? ================ Comment at: clang-tools-extra/docs/clang-tidy/checks/misc-assignment-in-if-clause.rst:6-9 +Finds assignments within the condition of `if` statements. +Allows automatic identification of assignments which may have been intended as equality tests. +Finds these assignments even within multiple sets of parentheses which is often appropriate to structure multi-part condition statements. +Finds these assignments even within multiple sets of paretheses which disables the compiler -Wparentheses check which one would otherwise like to rely on to find accidental assignment. ---------------- ================ Comment at: clang-tools-extra/docs/clang-tidy/checks/misc-assignment-in-if-clause.rst:9 +Finds these assignments even within multiple sets of parentheses which is often appropriate to structure multi-part condition statements. +Finds these assignments even within multiple sets of paretheses which disables the compiler -Wparentheses check which one would otherwise like to rely on to find accidental assignment. +The identified assignments violate BARR group "Rule 8.2.c". See: https://barrgroup.com/embedded-systems/books/embedded-c-coding-standard/statement-rules/if-else-statements ---------------- dodohand wrote: > gribozavr2 wrote: > > Why not improve `-Wparentheses` to catch these cases too? > IMHO, `-Wparentheses` isn't broken. Its "disable warning by an extra set of > parentheses" behavior has been around for a long time and is common across > gcc and clang... it just isn't what is called for when trying to ensure that > accidental assignments aren't occurring in even slightly complicated > condition statements, or when trying to comply with BARR-group coding > standard. Oh, I see -- it wasn't clear to me that the silencing through the extra set of parens was specifically not desirable for you. I suggested some edits to the documentation to directly highlight the difference. I also suggest changing the example below to explicitly mention what assignments are flagged by `-Wparentheses` and which are flagged by this check. ================ Comment at: clang-tools-extra/docs/clang-tidy/checks/misc-assignment-in-if-clause.rst:10 +Finds these assignments even within multiple sets of paretheses which disables the compiler -Wparentheses check which one would otherwise like to rely on to find accidental assignment. +The identified assignments violate BARR group "Rule 8.2.c". See: https://barrgroup.com/embedded-systems/books/embedded-c-coding-standard/statement-rules/if-else-statements + ---------------- ================ Comment at: clang-tools-extra/docs/clang-tidy/checks/misc-assignment-in-if-clause.rst:17 + + static volatile int f = 3; + if( f = 4 ) // This is identified - should it have been: `if ( f == 4 )` ? ---------------- The "static volatile" part is not relevant to the example, and it might distract the reader, suggesting that something is special about such variables. ================ Comment at: clang-tools-extra/test/clang-tidy/checkers/misc-assignment-in-if-clause.cpp:68 + d = val + 1; + return d; + } ---------------- Please clang-format the test. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D127114/new/ https://reviews.llvm.org/D127114 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
