[PATCH] D127114: new clang-tidy checker for assignments within condition clause of if statement

[Dmitri Gribenko via Phabricator via cfe-commits]

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