This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] Treat AttributeMacros more like __attribute__
ClosedPublic

Authored by jaredgrubb on Mar 3 2023, 12:01 PM.

Details

Summary

I noticed that clang-format was inserting some strange indentation whenever I used custom "attribute-like macros"
(things like FOO_EXTERN to wrap attribute-visible-default, or macros with parentheses like NS_SWIFT_NAME(...)).

There are two parts to this fix:

  • Annotate the paren after an AttributeMacro as an AttributeLParen.
  • Treat an AttributeMacro-without-paren the same as one with a paren.

I added a new test-case to differentiate a macro that is or is-not an AttributeMacro; also handled whether ColumnLimit is set to infinite (0) or a finite value, as part of this patch is in ContinuationIndenter.

There may be other places that need to handle TT_AttributeMacro better, but this is at least a step in the right
direction.

Closes https://github.com/llvm/llvm-project/issues/68722.

Diff Detail

Event Timeline

jaredgrubb created this revision.Mar 3 2023, 12:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 3 2023, 12:01 PM
jaredgrubb requested review of this revision.Mar 3 2023, 12:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 3 2023, 12:01 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Our tests reside in clang/unittests/Format, you want to look into the TokenAnnotatorTests. In fact I can't read the tests you wrote there.

And could you please provide an example what you want to format and where the misformatting (or apparently misannotation) happens?

For background, the current clang-format results in the following (-style="{BasedOnStyle: LLVM, ColumnLimit: 0, AttributeMacros: [MACRO]}):

MACRO MACRO(A)
    @interface Foo
@end

MACRO(A)
MACRO
    @interface Foo
@end

This patch improves it (removes the indention and makes both cases have same wrapping):

MACRO MACRO(A)
@interface Foo
@end

MACRO(A) MACRO
@interface Foo
@end

I wasn't sure about testing (this is my first patch!) and the test-case I did was in clang/test/Format .. I can look at clang/unittests/Format and see how to model something like it.

If I do that, would that be in-addition-to or instead-of the test-case I included already?

I wasn't sure about testing (this is my first patch!) and the test-case I did was in clang/test/Format .. I can look at clang/unittests/Format and see how to model something like it.

If I do that, would that be in-addition-to or instead-of the test-case I included already?

Instead of.

jaredgrubb updated this revision to Diff 502390.Mar 4 2023, 2:53 PM

Create unit-tests for the patch (and remove the proposed non-unit test "test").

clang/unittests/Format/FormatTestObjC.cpp
1528

So there is most likely a CanBreak not true? That happens also in the TokenAnnotator.

clang/unittests/Format/TokenAnnotatorTest.cpp
1325

Your bug is in regard to Objective-C, right? Maybe add a test for that here too?

  • Fixed an issue in TokenAnnotator about it not breaking between macros properly (it was catching in an ObjC selector-check too early)
  • Add more ObjC tests, covering method and property declarations too. There are still some quirks about reflowing multiple attributes, but those quirks exist in C++ too, so I think those are best left for another patch. I added checks for existing behavior so that patch can improve the ObjC version too.
jaredgrubb marked 2 inline comments as done.Mar 12 2023, 4:42 PM
jaredgrubb added inline comments.Mar 12 2023, 4:50 PM
clang/lib/Format/TokenAnnotator.cpp
5329

I broke this if into two pieces and restored an original return true for this part of the if that was changed in 2020 (via 5a4ddbd69db2b0e09398214510501d0e59a0c30b).

The return !Left... was added to avoid breaking [[ apart when reflowing a function-arg with C++11-style attribute (eg, [[unused]]). As far as I can tell, this check _really_ only matters if the code-path is the second-half of the original if (Right.is(tok::l_square)...).

I couldn't imagine how there Right.is(tok::kw___attribute) needs the Left-check, so I think that patch is better split to differentiate (even though originally it didn't really have any conflicting effect).

Please let me know if my logic here is wrong (or I'm not making sense :) )

jaredgrubb added inline comments.Mar 12 2023, 4:59 PM
clang/unittests/Format/FormatTestObjC.cpp
1619

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!)

owenpan added inline comments.Mar 23 2023, 11:10 PM
clang/lib/Format/TokenAnnotator.cpp
4380
clang/unittests/Format/FormatTestObjC.cpp
1682–1693
clang/unittests/Format/TokenAnnotatorTest.cpp
1307–1313

I would delete it.

1335–1342

Ditto.

1364–1371

Ditto.

1393–1400

Ditto.

jaredgrubb marked 6 inline comments as done.

Address review comments from @owenpan

I have uploaded patch for all the points you called out. (Sorry for delay; I missed the suggestions earlier!)

MyDeveloperDay added inline comments.Apr 24 2023, 2:00 AM
clang/lib/Format/ContinuationIndenter.h
231

If you want this to look like code you need to use \code \endcode

MyDeveloperDay added inline comments.Apr 24 2023, 2:02 AM
clang/lib/Format/ContinuationIndenter.h
231

Ok my bad this is the ContinuationIndenter.h, not Format.h

Address review feedback about code/endcode. Otherwise the patch is the same and I hope ready for merge? I'd love to get a green check :)

jaredgrubb marked 2 inline comments as done.May 7 2023, 10:16 AM
clang/lib/Format/TokenAnnotator.cpp
4380–4381
5325

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)?

jaredgrubb added inline comments.May 7 2023, 10:47 PM
clang/lib/Format/TokenAnnotator.cpp
5325

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));
clang/lib/Format/TokenAnnotator.cpp
5325

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.

jaredgrubb marked an inline comment as done.May 13 2023, 3:13 PM
jaredgrubb added inline comments.
clang/lib/Format/TokenAnnotator.cpp
5325

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.

Address review comments:

  • remove redundant &&
  • remove part of patch that was not tested by any test and should be its own patch on its own merits
jaredgrubb marked 2 inline comments as done.May 13 2023, 3:18 PM
MyDeveloperDay accepted this revision.May 14 2023, 8:26 AM
This revision is now accepted and ready to land.May 14 2023, 8:26 AM

Rebased and added a line to Release Notes, no other changes. Have requested this to be merged if all builds are green (since this patch has been approved).

owenpan added inline comments.Jul 14 2023, 2:29 PM
clang/lib/Format/ContinuationIndenter.h
231

Unrelated.

234

Ditto.

clang/lib/Format/TokenAnnotator.cpp
5324–5329

I'd delete the comment on line 5519.

clang/unittests/Format/FormatTestObjC.cpp
1619

Half of the test cases passed before this patch but now would fail with this patch. That is, this patch would generate regressions.

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.

Rebased.
Removed extraneous doc comment not related to patch (as requested).
Removed comment (as requested).
Added a #if 0 around some cases that should be fixed one day (as requested prior but I missed one of the chunks).

jaredgrubb marked an inline comment as done.Sep 23 2023, 3:17 PM
jaredgrubb added inline comments.
clang/unittests/Format/FormatTestObjC.cpp
1619

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.

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

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.

clang/docs/ReleaseNotes.rst
474–475 ↗(On Diff #557274)

Can you delete this as release notes are usually for new options?

clang/lib/Format/TokenAnnotator.cpp
5324–5326
clang/unittests/Format/FormatTestObjC.cpp
1619

I'm okay with fixing them in another patch.

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

I have merged the PR. Commits cef9f40cd4fe and 151d0a4db321 should make this patch less brittle.

jaredgrubb updated this revision to Diff 557542.Oct 2 2023, 3:36 PM

Address review comments, and adjusting the patch to address other merges since this patch was started.

jaredgrubb marked an inline comment as done.Oct 2 2023, 3:37 PM
owenpan edited the summary of this revision. (Show Details)Oct 2 2023, 4:42 PM
owenpan edited the summary of this revision. (Show Details)Oct 2 2023, 4:48 PM
owenpan added inline comments.Oct 2 2023, 9:01 PM
clang/lib/Format/ContinuationIndenter.cpp
1230–1234

Basically, insert the first two lines and undo the changes to the last three lines.

clang/lib/Format/TokenAnnotator.cpp
4085–4086

Please also add a test case for this.

4379–4381

Please delete this comment, which IMO doesn't quite match the code below.

owenpan retitled this revision from [clang-format] Treat AttributeMacros more like attribute macros to [clang-format] Treat AttributeMacros more like __attribute__.Oct 2 2023, 11:10 PM

Can you add a github issue to show the behavior before this patch?

Addressed review feedback from Oct2 (@owenpan).
A new issue on Github was opened for this enhancement (https://github.com/llvm/llvm-project/issues/68722)

jaredgrubb marked 5 inline comments as done.Oct 10 2023, 9:25 AM
owenpan edited the summary of this revision. (Show Details)Oct 10 2023, 2:05 PM
owenpan accepted this revision.Oct 10 2023, 2:11 PM

LGTM

@jaredgrubb please provide your authorship for git commit --author so that we can land the patch on your behalf.

@jaredgrubb please provide your authorship for git commit --author so that we can land the patch on your behalf.

The best to use would be:

Jared Grubb <jgrubb@apple.com>

Thanks so much for your help on this!!

This revision was automatically updated to reflect the committed changes.