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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
