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

[Owen Pan via Phabricator via cfe-commits]

owenpan added a comment.

This looks OK to me. Please rebase after https://github.com/llvm/llvm-project/pull/67518 is merged.

In D145262#4650300 <https://reviews.llvm.org/D145262#4650300>, @jaredgrubb wrote:

> Hopefully this will satisfy and we can merge! I didn't notice the Phabricator timeline and I'm hoping we can finish this before Oct 1.

The new deadline is Nov 15 according to https://discourse.llvm.org/t/update-on-github-pull-requests/71540/125.



================
Comment at: clang/docs/ReleaseNotes.rst:474-475
 - Add ``AllowBreakBeforeNoexceptSpecifier`` option.
+- Improve handling of ``AttributeMacros`` to behave more consistently like
+  regular attribute macros (aka ``__attribute__``)
 
----------------
Can you delete this as release notes are usually for new options?


================
Comment at: clang/lib/Format/TokenAnnotator.cpp:5608-5610
+    return Left.isNot(TT_AttributeSquare);
+
+  // Don't split `[[` on C++ attributes.
----------------



================
Comment at: clang/unittests/Format/FormatTestObjC.cpp:1619
+  // Reflow after first macro.
+  // FIXME: these should indent but don't.
+  verifyFormat("- (id)init ATTRIBUTE_MACRO(X)\n"
----------------
jaredgrubb wrote:
> owenpan wrote:
> > jaredgrubb wrote:
> > > I don't love this FIXME, but I was afraid to add more to this patch, as fixing this will require digging into things that have nothing to do with `__attribute__` vs `AttributeMacros`.
> > > 
> > > For example, suffix macros in C/C++ also are broken in the same way with just plain `__attribute__`. For example, for `ColumnWidth: 50`:
> > > ```
> > > int f(double) __attribute__((overloadable))
> > > __attribute__((overloadable));
> > > 
> > > int ffffffffffffffffffffffffffffff(double)
> > >     __attribute__((overloadable))
> > >     __attribute__((overloadable));
> > > ```
> > > 
> > > I think fixing reflowing of suffix macros is best done in another PR (which I can take a stab at!)
> > Half of the test cases passed before this patch but now would fail with this patch. That is, this patch would generate regressions.
> Just saw this comment. Yes, 3 of these did pass, but lots more in this file do NOT pass. I understand the desire to not "regress", but this patch improves so many other examples (as documented in these test cases). I can pick some of the worst if it helps, but otherwise, I'm not sure what I can do to address your comment.
I'm okay with fixing them in another patch.


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

https://reviews.llvm.org/D145262