[PATCH] D44994: [clang-format] Ensure wrapped ObjC selectors with 1 arg obey IndentWrappedFunctionNames

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton added a comment.

Thanks, applied your suggested change.



================
Comment at: lib/Format/ContinuationIndenter.cpp:899
     if (!State.Stack.back().ObjCSelectorNameFound) {
+      unsigned MinIndent =
+          (Style.IndentWrappedFunctionNames
----------------
djasper wrote:
> I think I'd now find this slightly easier to read as:
> 
>   unsigned MinIndent = State.Stack.back().Indent;
>   if (Style.IndentWrappedFunctionNames)
>     MinIndent = std::max(MinIndent, State.FirstIndent + Style.ContinuationIndentWidth);
OK, done.


================
Comment at: lib/Format/ContinuationIndenter.cpp:904
+               : State.Stack.back().Indent);
       if (NextNonComment->LongestObjCSelectorName == 0)
+        return MinIndent;
----------------
djasper wrote:
> Does this if actually change the behavior in any way? With LongestObjCSelectorName being 0, isn't that:
> 
>   return MinIndent +
>          std::max(0, ColumnWidth) - ColumnWidth;
> 
> (and with ColumnWidth being >= 0, this should be just MinIndent)
The `- ColumnWidth` part is only for the case where `LongestObjCSelectorName` is *not* 0. If it's 0, we return `MinIndent` which ensures we obey `Style.IndentWrappedFunctionNames`.

The problem with the code before this diff is when `LongestObjCSelectorName` was 0, we ignored `Style.IndentWrappedFunctionNames` and always returned `State.Stack.back().Indent` regardless of that setting.

After this diff, when `LongestObjCSelectorName` is 0 (i.e., this is either the first part of the selector or a selector which is not colon-aligned due to block formatting), we change the behavior to indent to at least `State.FirstIndent + Style.ContinuationIndentWidth`, like all other indentation logic in this file.

I've added some comments explaining what's going on, since this code is quite complex.


Repository:
  rC Clang

https://reviews.llvm.org/D44994