[clang-format] Treat AttributeMacros more like attribute macros

[jaredgrubb]

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.

[HazardyKnusperkeks]

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?

[jaredgrubb]

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

[jaredgrubb]

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?

[owenpan]

In D145262#4168058, @jaredgrubb wrote:

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]

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

[HazardyKnusperkeks]

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?

[jaredgrubb]

• 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]

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]

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]

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]

Address review comments from @owenpan

[jaredgrubb]

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

[MyDeveloperDay]

clang/lib/Format/ContinuationIndenter.h

231

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

[MyDeveloperDay]

clang/lib/Format/ContinuationIndenter.h

231

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

[jaredgrubb]

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

[HazardyKnusperkeks]

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]

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

[HazardyKnusperkeks]

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]

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.

[jaredgrubb]

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