[PATCH] D71239: [clang-format] Fix ObjC keywords following try/catch getting split.

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.


================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:1849
+      } while (FormatTok->is(tok::comment));
+    }
     if (!(FormatTok->isOneOf(tok::kw_catch, Keywords.kw___except,
----------------
Bigcheese wrote:
> MyDeveloperDay wrote:
> > can you use `FormatTok->getNextNonComment()`?
> No, `Next` has not been setup at this point, so it will always return `nullptr`.
There is something about what we are doing here which doesn't feel correct...If you look in FormatTokenLexer why are we not doing something similar here for @try and @catch as we are doing for @"ABC" (see below tryMergeNSStringLiteral)

The FormatTokenLexer can merge together a number of tokens into a new single token and combined text, after that nothing can split that token, meaning the `@` and `try` will never become separated and will simply be treated as a "@try"

This can be useful because you can set the new setKind() to be "tok::try" after which `@try` will behave just like a normal `try` for all rules without the constant need to keep checking if the previous token is an `@`

```
bool FormatTokenLexer::tryMergeNSStringLiteral() {
  if (Tokens.size() < 2)
    return false;
  auto &At = *(Tokens.end() - 2);
  auto &String = *(Tokens.end() - 1);
  if (!At->is(tok::at) || !String->is(tok::string_literal))
    return false;
  At->Tok.setKind(tok::string_literal);
  At->TokenText = StringRef(At->TokenText.begin(),
                            String->TokenText.end() - At->TokenText.begin());
  At->ColumnWidth += String->ColumnWidth;
  At->Type = TT_ObjCStringLiteral;
  Tokens.erase(Tokens.end() - 1);
  return true;
}
```



================
Comment at: clang/unittests/Format/FormatTestObjC.cpp:200
+               "} @catch (NSException *e) {\n"
+               "}\n");
   verifyFormat("DEBUG({\n"
----------------
Bigcheese wrote:
> MyDeveloperDay wrote:
> > Nit: Could you not keep the original test? just add a new testcase? I get uncomfortable about changing tests no matter how trivial
> Is there something special about clang-format that causes this concern? I'm all for testing separate things separately, but the additions to this test are testing the same leaf lines of code.
as you are scanning back, I'd like to know we haven't broken the previous tests behaviour.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71239