[PATCH] D145262: [clang-format] Treat AttributeMacros more like attribute macros

[Jared Grubb via Phabricator via cfe-commits]

jaredgrubb marked an inline comment as done.
jaredgrubb added inline comments.


================
Comment at: clang/lib/Format/TokenAnnotator.cpp:5473
+  if (Right.isOneOf(tok::kw___attribute, TT_AttributeMacro))
+    return true;
+
----------------
HazardyKnusperkeks wrote:
> jaredgrubb wrote:
> > HazardyKnusperkeks wrote:
> > > Does changing this return value make no difference? In other words is there no combination of `Left.is(TT_AttributeSquare)` and `Right.is(tok::kw___attribute)`?
> > Yes, that combo can happen; example below.
> > 
> > The patch that changed the left-diff line from `true` to `!Left.is(TT_AttributeSquare)`
> >  was done _only_ contemplating the `[[` case (5a4ddbd69db2b0e09398214510501d0e59a0c30b); tagging @MyDeveloperDay who wrote that patch and can perhaps offer more insight on your question.
> > 
> > My reasoning is to revert that part of the patch just a bit and limit it to that case only.
> > 
> > I couldn't come up with a use-case where you'd want to avoid splitting between `TT_AttributeSquare` and `kw___attribute`, but the example below shows that allowing it to break in that combination is preferable to the alternative of breaking in the parens of the attribute:
> > ```
> > // Style: "{BasedOnStyle: LLVM, ColumnLimit: 40}"
> > // Existing Behavior
> > int ffffffffffffffffffffffffffffff(
> >     double)
> >     __attribute__((overloadable))
> >     [[unused]] __attribute__((
> >         overloadable));
> > 
> > // With Patch
> > int ffffffffffffffffffffffffffffff(
> >     double)
> >     __attribute__((overloadable))
> >     [[unused]]
> >     __attribute__((overloadable));
> > ```
> > 
> Thanks for the detailed answer. I'll wait for @MyDeveloperDay. You are right, it's prettier with the patch, but on the other hand it is not desirable to change the formatting (apart from fixing bugs) between versions.
This part of the patch isn't necessary so let me revert it so it doesn't slow this patch down.

But, the unit tests don't even fail when I revert it -- which means I really should treat this part as its own change with a unit test that matters. 


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

https://reviews.llvm.org/D145262