Refactor: Simplify boolean expressions in lib/Parse

[LegalizeAdulthood]

Summary

Simplify boolean expressions using true and false with clang-tidy

[rjmccall]

I dunno, I think it like it better the old way. It's a big, ugly condition either way, and the old way leaves more space for further big, ugly conditions.

[majnemer]

lib/Parse/ParseDecl.cpp

5191–5193

I think these sentences need to be rephrased now that the control flow has been removed.

5198

I think you are missing // 'int([[]]int)' is a function. on this line.

[LegalizeAdulthood]

lib/Parse/ParseDecl.cpp

5198

Thanks, I did miss that comment. clang-tidy isn't preserving the comments on this check (I might enhance this new check further to do that), so I was backfilling the comments in by hand and I missed one.

When I looked at this diff, I was thinking that this large, complicated boolean expression might be more understandable if it were broken down with some intention-revealing local variables; something like:

} else {
  const bool FunctionReturnsInt = Tok.is(tok::r_paren);
  const bool VariadicFunctionReturnsInt = getLangOpts().CPlusPlus && Tok.is(tok::ellipsis) && NextToken().is(tok::r_paren);
  isGrouping = !(FunctionReturnsInt || VariadicFunctionReturnsInt || isDeclarationSpecifier() || isCXX11AttributeSpecifier());
}

The original author of this code seems to understand the complexity here, hence the sprinkling of comments to explain the logic. My own style is to prefer intention revealing names instead of comments, but my preferences are not the issue here. I recognized that the comments were important here, so I tried to preserve them in my diff. I'm not sure how the full-line comments should be reworded to express the same intent, so I left them as is. I'm happy to adjust them if someone can suggest an alternative text.

[rjmccall]

lib/Parse/ParseDecl.cpp

5198

A more readable structure would be:

// 'int()' is a function.
} else if (Tok.is(tok::r_paren)) {
  isGrouping = false;

// C++ 'int(...)' is a function.
} else if (getLangOpts().CPlusPlus && Tok.is(tok::ellipsis) && NextToken().is(tok::r_paren)) {
  isGrouping = false;

// 'int(int)' is a function.
} else if (isDeclarationSpecifier()) {
  isGrouping = false;

// C++11 'int([[...]])' is a function.
} else if (isCXX11AttributeSpecifier()) {
  isGrouping = false;

// Otherwise, it's a grouping parenthesis.
} else {
  isGrouping = true;
}

Note that putting the condition in an assignment to isGrouping is still not helpful.

[rsmith]

We have three cases here:

• do we require a name? if so, it's a grouping paren
• do we have a (possibly empty) list of parameters? if so, it's not a grouping paren
• otherwise, it's a grouping paren

It doesn't make sense to combine the second and third cases together. If this code needs clarifying, one way to do it would be to factor out the second check into an appropriately-named function.

[LegalizeAdulthood]

Thanks for all the comments on this. This is exactly why I broke up these changes into one per module of related code :-).

In D8530#145602, @rsmith wrote:

We have three cases here:

• do we require a name? if so, it's a grouping paren
• do we have a (possibly empty) list of parameters? if so, it's not a grouping paren
• otherwise, it's a grouping paren

It doesn't make sense to combine the second and third cases together. If this code needs clarifying, one way to do it would be to factor out the second check into an appropriately-named function.

How would you feel about Extract Method isGroupingParen that looked like this:

bool Parser::isGroupingParen(const Declarator &D) {
  if (!D.mayOmitIdentifier()) {
    // If this can't be an abstract-declarator, this *must* be a grouping
    // paren, because we haven't seen the identifier yet.
    return true;
  }

  if (Tok.is(tok::r_paren) ||             // 'int()' is a function.
      (getLangOpts().CPlusPlus && Tok.is(tok::ellipsis) &&
       NextToken().is(tok::r_paren)) ||   // C++ int(...)
      isDeclarationSpecifier() ||         // 'int(int)' is a function.
      isCXX11AttributeSpecifier()) {      // 'int([[]]int)' is a function.
    // This handles C99 6.7.5.3p11: in "typedef int X; void foo(X)", X is
    // considered to be a type, not a K&R identifier-list.
    return false;
  }

  // Otherwise, this is a grouping paren, e.g. 'int (*X)' or 'int(X)'.
  return true;
}

and the call point then becomes:

// If we haven't past the identifier yet (or where the identifier would be
// stored, if this is an abstract declarator), then this is probably just
// grouping parens. However, if this could be an abstract-declarator, then
// this could also be the start of function arguments (consider 'void()').

// If this is a grouping paren, handle:
// direct-declarator: '(' declarator ')'
// direct-declarator: '(' attributes declarator ')'
if (isGroupingParen(D)) {

[LegalizeAdulthood]

Abandoning this changeset after improving clang-tidy to handle chained conditional assignment.

lib/Parse/ParseDecl.cpp

void Parser::ParseParenDeclarator(Declarator &D) {
   // grouping parens. However, if this could be an abstract-declarator, then
   // this could also be the start of function arguments (consider 'void()').
   bool isGrouping;
 
   if (!D.mayOmitIdentifier()) {
     // If this can't be an abstract-declarator, this *must* be a grouping
     // paren, because we haven't seen the identifier yet.
     isGrouping = true;
-  } else if (Tok.is(tok::r_paren) ||           // 'int()' is a function.
-             (getLangOpts().CPlusPlus && Tok.is(tok::ellipsis) &&
-              NextToken().is(tok::r_paren)) || // C++ int(...)
-             isDeclarationSpecifier() ||       // 'int(int)' is a function.
-             isCXX11AttributeSpecifier()) {    // 'int([[]]int)' is a function.
+  } else
     // This handles C99 6.7.5.3p11: in "typedef int X; void foo(X)", X is
     // considered to be a type, not a K&R identifier-list.
-    isGrouping = false;
-  } else {
     // Otherwise, this is a grouping paren, e.g. 'int (*X)' or 'int(X)'.
-    isGrouping = true;
-  }
+    isGrouping = !(Tok.is(tok::r_paren) || // 'int()' is a function.
+                   (getLangOpts().CPlusPlus && Tok.is(tok::ellipsis) &&
+                    NextToken().is(tok::r_paren)) || // C++ int(...)
+                   isDeclarationSpecifier() || // 'int(int)' is a function.
+                   isCXX11AttributeSpecifier());
 
   // If this is a grouping paren, handle:
   // direct-declarator: '(' declarator ')'
   // direct-declarator: '(' attributes declarator ')'
   if (isGrouping) {
     SourceLocation EllipsisLoc = D.getEllipsisLoc();
     D.setEllipsisLoc(SourceLocation());