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

[dodohand via Phabricator via cfe-commits]

dodohand 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.");
----------------
gribozavr2 wrote:
> 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.
Will adjust the messages.

I'm specifically reluctant to add a fixit to this, as I'm not sure how I can tell an intended assignment (for which the fix would be moving the assignment out of the if clause) apart from an accidental assignment (for which the fix is changing '=' to '=='). Thoughts?


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/misc-assignment-in-if-clause.rst:3
+
+misc-assignment-in-if-clause
+============================
----------------
gribozavr2 wrote:
> WDYT about `bugprone-assignment-in-if-condition`?
I'm in favor of that categorization - it reflects how I feel about having assignments within an 'if' condition. I'll change it accordingly


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127114/new/

https://reviews.llvm.org/D127114