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

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added inline comments.


================
Comment at: clang/lib/Format/TokenAnnotator.cpp:5473
+  if (Right.isOneOf(tok::kw___attribute, TT_AttributeMacro))
+    return true;
+
----------------
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.


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

https://reviews.llvm.org/D145262