[PATCH] D108094: [clang-format] Improve detection of parameter declarations in K&R C

[Owen Pan via Phabricator via cfe-commits]

owenpan added inline comments.


================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:1025
   if (!isC78Type(*Tok) &&
       !Tok->isOneOf(tok::kw_register, tok::kw_struct, tok::kw_union))
     return false;
----------------
MyDeveloperDay wrote:
> should we have test cases showing these examples?
Probably not unless we are to include a test case for every keyword. I can add them if you insist.


================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:1029
+  if (Next->isNot(tok::star) && !Next->Tok.getIdentifierInfo())
+    return false;
+
----------------
MyDeveloperDay wrote:
> Can you add unit tests for these cases?
I think the existing test cases already cover this, which is a subset of the previous cases (the second token can be anything but `tok::l_paren` and `tok::semi`)?


================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:1397
       assert(Position < AllTokens.size());
-      const FormatToken *Next = AllTokens[Position];
-      if (Next && Next->isOneOf(tok::l_paren, tok::semi))
-        break;
-      if (isC78ParameterDecl(FormatTok)) {
+      if (isC78ParameterDecl(FormatTok, AllTokens[Position], Previous)) {
         addUnwrappedLine();
----------------
MyDeveloperDay wrote:
> Can't next be null once again?
No because `FormatTok` is not `tok::eof`. If `AllTokens[Position]` is null, it would be an internal error, which is asserted at the top of `isC78ParameterDecl()`.


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

https://reviews.llvm.org/D108094