[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

sorry to be critical, just trying to help get traction on your review...



================
Comment at: lib/Format/TokenAnnotator.cpp:65
 
-    const FormatToken &Previous = *CurrentToken->Previous;  // The '<'.
+    const FormatToken &Previous = *CurrentToken->Previous; // The '<'.
     if (Previous.Previous) {
----------------
unrelated change, consider backing out, perhaps we could do a clang-format of the file as a separate pass


================
Comment at: lib/Format/TokenAnnotator.cpp:526
         if (!Contexts.back().FirstObjCSelectorName) {
-            FormatToken* Previous = CurrentToken->getPreviousNonComment();
-            if (Previous && Previous->is(TT_SelectorName)) {
-              Previous->ObjCSelectorNameParts = 1;
-              Contexts.back().FirstObjCSelectorName = Previous;
-            }
+          FormatToken *Previous = CurrentToken->getPreviousNonComment();
+          if (Previous && Previous->is(TT_SelectorName)) {
----------------
unrelated change, consider backing out, perhaps we could do a clang-format of the file as a separate pass


================
Comment at: lib/Format/TokenAnnotator.cpp:1392
+               // FIXME(bug 36976): ObjC return types shouldn't use
+               // TT_CastRParen.
                Current.Previous && Current.Previous->is(TT_CastRParen) &&
----------------
unrelated change, consider backing out, perhaps we could do a clang-format of the file as a separate pass


================
Comment at: lib/Format/TokenAnnotator.cpp:2420
+                                Right.MatchingParen->Next &&
+                                Right.MatchingParen->Next->is(tok::colon);
     return !IsLightweightGeneric && Style.ObjCSpaceBeforeProtocolList;
----------------
unrelated change, consider backing out, perhaps we could do a clang-format of the file as a separate pass


================
Comment at: lib/Format/TokenAnnotator.cpp:2545
+              Left.Tok.getIdentifierInfo()->isKeyword(
+                  getFormattingLangOpts(Style)))) &&
+            Line.Type != LT_PreprocessorDirective) ||
----------------
would be nice to pull the 3 items into a function because you repeat it on line 2551


================
Comment at: lib/Format/TokenAnnotator.cpp:2548
+           (Style.SpaceBeforeParens == FormatStyle::SBPO_NonEmptyParentheses &&
+            Right.ParameterCount >= 1 &&
+            (Left.is(tok::identifier) || Left.isFunctionLikeKeyword() ||
----------------
something similar here when combined with line 2540, looks a lot like the 2 patterns below?

almost like its:


```
bool allowSpaceBeforeIfFunctionHasParameters(Style,Right);
```


================
Comment at: lib/Format/TokenAnnotator.cpp:2617
+    if (Left.MatchingParen &&
+        Left.MatchingParen->is(TT_ProtoExtensionLSquare) &&
         Right.isOneOf(tok::l_brace, tok::less))
----------------
unrelated change, consider backing out, perhaps we could do a clang-format of the file as a separate pass


================
Comment at: lib/Format/TokenAnnotator.cpp:2833
+           (Style.SpaceBeforeParens == FormatStyle::SBPO_NonEmptyParentheses &&
+            Right.ParameterCount >= 1);
   if (Right.is(TT_TemplateOpener) && Left.is(tok::r_paren) &&
----------------
can this and the above section on line 2745 be combined? I'm not sure if the order of the "if" indicates precedence


================
Comment at: lib/Format/TokenAnnotator.cpp:3138
     if (Right.is(Keywords.kw_is)) {
-      const FormatToken* Next = Right.getNextNonComment();
+      const FormatToken *Next = Right.getNextNonComment();
       // If `is` is followed by a colon, it's likely that it's a dict key, so
----------------
unrelated change, consider backing out, perhaps we could do a clang-format of the file as a separate pass


Repository:
  rC Clang

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

https://reviews.llvm.org/D55170