[PATCH] D56303: [clang-tidy] Recognize labelled statements when simplifying boolean exprs

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D56303#3270090 <https://reviews.llvm.org/D56303#3270090>, @LegalizeAdulthood wrote:

> In D56303#3266252 <https://reviews.llvm.org/D56303#3266252>, @njames93 wrote:
>
>> A large portion of the changes, especially in the checks implementation file, appear to be NFC stylistic or formatting only fixes. While these changes are generally good, they shouldn't be a part of this patch. Instead they should be committed in their own NFC patch.
>> This makes it much easier to review the relevant changes needed to implement this new behaviour.
>
> It's going to be a significant amount of work to tease everything
> apart and this patch has already been waiting literally for years.

I don't think you need to tease it apart for this patch, but FWIW, I agree that this is much harder to review in this state.

> Every time I put something up for review I get told "do this because
> the style guide says so", so I anticipated those things and fixed them
> pre-emptively.

The rule of thumb is: if you have significant NFC changes, do them first and push the changes (often doesn't even require a review because it's NFC, but that depends on what comfort level you have about the changes being NFC), and then do the functional changes on top of the improved code. Reviewers will ask you to update style for the lines of code you're touching when appropriate, but we (generally) shouldn't be asking you to fix style issues in code you're not touching. (Oddly, we touch on this in #2 at https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access which is not where I'd expect that information to live.)

FWIW, I looked through the changes and don't have major concerns. A second set of eyes approving this would be appreciated, but this LGTM.



================
Comment at: clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp:72-73
 
-const Expr *getBoolLiteral(const MatchFinder::MatchResult &Result,
-                           StringRef Id) {
+static const Expr *getBoolLiteral(const MatchFinder::MatchResult &Result,
+                                  StringRef Id) {
   if (const Expr *Literal = Result.Nodes.getNodeAs<CXXBoolLiteralExpr>(Id))
----------------
LegalizeAdulthood wrote:
> LegalizeAdulthood wrote:
> > FYI
> > 
> > These tabs here (and elsewhere) are phantoms somehow
> > introduced into the diff by Phabricator.  My local diff file
> > that I uploaded has zero tabs in it anywhere.
> > 
> When I look more carefully, I see it's one column to the left of the
> first column in the file.... so it's trying to somehow say that this line
> had increased indent in the diff?
> 
> I thought it was a TAB indicator and another reviewer thought so as well.
> 
> But it isn't a TAB at all `:)`
Yeah, that's caught me a few times as well. The `>>` seems to mean "indentation changed" not "tab inserted".


================
Comment at: clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp:422
 
-  switch (Op->getOpcode()) {
-    case BO_LAnd:
-      if (BoolValue) {
-        // expr && true -> expr
-        ReplaceWithExpression(Other, /*Negated=*/false);
-      } else {
-        // expr && false -> false
-        ReplaceWithExpression(Bool, /*Negated=*/false);
-      }
-      break;
-    case BO_LOr:
-      if (BoolValue) {
-        // expr || true -> true
-        ReplaceWithExpression(Bool, /*Negated=*/false);
-      } else {
-        // expr || false -> expr
-        ReplaceWithExpression(Other, /*Negated=*/false);
-      }
-      break;
-    case BO_EQ:
-      // expr == true -> expr, expr == false -> !expr
-      ReplaceWithExpression(Other, /*Negated=*/!BoolValue);
-      break;
-    case BO_NE:
-      // expr != true -> !expr, expr != false -> expr
-      ReplaceWithExpression(Other, /*Negated=*/BoolValue);
-      break;
-    default:
-      break;
+  switch (Op->getOpcode()) { // NOLINT(clang-diagnostic-switch-enum)
+  case BO_LAnd:
----------------
I'd prefer the `NOLINT` comment be removed (we don't typically add those, same as clang-format comments or other special markings).


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

https://reviews.llvm.org/D56303