[PATCH] D119785: [clang-format] Fix formatting of struct-like records followed by variable declaration.

[Marek Kurdej via Phabricator via cfe-commits]

curdeius marked 3 inline comments as done.
curdeius added inline comments.


================
Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:735
         // We don't merge short records.
-        FormatToken *RecordTok = Line.First;
-        // Skip record modifiers.
-        while (RecordTok->Next &&
-               RecordTok->isOneOf(tok::kw_typedef, tok::kw_export,
-                                  Keywords.kw_declare, Keywords.kw_abstract,
-                                  tok::kw_default, Keywords.kw_override,
-                                  tok::kw_public, tok::kw_private,
-                                  tok::kw_protected, Keywords.kw_internal))
-          RecordTok = RecordTok->Next;
-        if (RecordTok &&
-            RecordTok->isOneOf(tok::kw_class, tok::kw_union, tok::kw_struct,
-                               Keywords.kw_interface))
+        if (isRecordLBrace(*Line.Last))
           return 0;
----------------
HazardyKnusperkeks wrote:
> curdeius wrote:
> > curdeius wrote:
> > > MyDeveloperDay wrote:
> > > > wow these are equivalent? do we need to worry about trailing comments?
> > > > 
> > > > ```
> > > > public class A { /* comment */
> > > > ```
> > > Yes, before we were doing a poor man's scan skipping some (but not all) keywords (hence the bugs) that could appear before a record.
> > > That was error-prone and redundant w.r.t. the parsing done in UnwrappedLineParser.
> > > 
> > > Anyway, good catch, I'll test what happens with comments.
> > @MyDeveloperDay, do we want this (probably more coherent):
> > ```
> >   verifyFormat("struct A {};", AllowSimpleBracedStatements); // already tested
> >   verifyFormat("struct A { /* comment */ };", AllowSimpleBracedStatements); // added
> > ```
> > or this (current behaviour):
> > ```
> >   verifyFormat("struct A {};", AllowSimpleBracedStatements); // already tested
> >   verifyFormat("struct A { /* comment */\n"
> >                        "};", AllowSimpleBracedStatements); // added
> > ```
> > 
> > (adapted from `TEST_F(FormatTest, FormatShortBracedStatements)`)
> > @MyDeveloperDay, do we want this (probably more coherent):
> > ```
> >   verifyFormat("struct A {};", AllowSimpleBracedStatements); // already tested
> >   verifyFormat("struct A { /* comment */ };", AllowSimpleBracedStatements); // added
> > ```
> > or this (current behaviour):
> > ```
> >   verifyFormat("struct A {};", AllowSimpleBracedStatements); // already tested
> >   verifyFormat("struct A { /* comment */\n"
> >                        "};", AllowSimpleBracedStatements); // added
> > ```
> > 
> > (adapted from `TEST_F(FormatTest, FormatShortBracedStatements)`)
> 
> Ideally I'd say depending on the input. It not dependent on that, the former.
So, I did some testing.
First of all, I'd rather keep the current behaviour, otherwise it'll be a breaking change (for those that have such a case).
We can always do it later if need be (but I doubt it's important).

> do we need to worry about trailing comments?

We don't, in `tryMergeSimpleBlock`, such comments are already on the next line so `Line.Last` is correctly pointing to the opening brace `{`.
Anyway, going to land as is (+ a new test case).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119785