Page MenuHomePhabricator

[clang-format] Treat AttributeMacros more like attribute macros
AcceptedPublic

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:

  • tokenize the paren after an AttributeMacro as a TT_AttributeParen
  • treat a AttributeMacro-without-paren the same as one with a paren (eg, the FOO_EXTERN case)

I added a new test-case to differentiate a macro that is or is-not a AttributeMacro; also handled whether the
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.

Diff Detail

Unit TestsFailed

TimeTest
4,400 msx64 debian > AddressSanitizer-x86_64-linux.TestCases/Linux::auto_memory_profile_test.cpp
Script: -- : 'RUN: at line 6'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -fsanitize=address -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -gline-tables-only -m64 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/asan/TestCases/Linux/auto_memory_profile_test.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/asan/X86_64LinuxConfig/TestCases/Linux/Output/auto_memory_profile_test.cpp.tmp

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
1533

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

clang/unittests/Format/TokenAnnotatorTest.cpp
1506

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
5477–5478

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
1624

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
4514
clang/unittests/Format/FormatTestObjC.cpp
1687–1698
clang/unittests/Format/TokenAnnotatorTest.cpp
1488–1494

I would delete it.

1516–1523

Ditto.

1545–1552

Ditto.

1574–1581

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.Sun, May 7, 10:16 AM
clang/lib/Format/TokenAnnotator.cpp
4514–4515
5473

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.Sun, May 7, 10:47 PM
clang/lib/Format/TokenAnnotator.cpp
5473

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
5473

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.Sat, May 13, 3:13 PM
jaredgrubb added inline comments.
clang/lib/Format/TokenAnnotator.cpp
5473

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.Sat, May 13, 3:18 PM
MyDeveloperDay accepted this revision.Sun, May 14, 8:26 AM
This revision is now accepted and ready to land.Sun, May 14, 8:26 AM