Page MenuHomePhabricator

clang-format: better handle namespace macros
ClosedPublic

Authored by Typz on Sep 13 2017, 9:27 AM.

Details

Summary

Other macros are used to declare namespaces, and should thus be handled
similarly. This is the case for crpcut's TESTSUITE macro, or for
unittest-cpp's SUITE macro:

TESTSUITE(Foo) {
TEST(MyFirstTest) {
  assert(0);
}
} // TESTSUITE(Foo)

This patch deals with this cases by introducing a new option to specify
lists of namespace macros. Internally, it re-uses the system already in
place for foreach and statement macros, to ensure there is no impact on
performance.

Diff Detail

Repository
rL LLVM

Event Timeline

Typz created this revision.Sep 13 2017, 9:27 AM
Typz added a reviewer: klimek.Sep 13 2017, 9:30 AM
klimek edited edge metadata.Sep 20 2017, 2:28 AM

I think instead of introducing more and more special cases of macros we might want to handle, we should instead allow specifying macro productions globally.

Typz added a comment.Oct 24 2017, 8:57 AM

I think instead of introducing more and more special cases of macros we might want to handle, we should instead allow specifying macro productions globally.

what do you mean?
Do you mean to group all the macro configuration options into "Macros" field, still containing one field for each kind of macro? Or do you have something else in mind?

In D37813#930065, @Typz wrote:

ping?

Argh, very sorry for the delay in response.

In D37813#905257, @Typz wrote:

I think instead of introducing more and more special cases of macros we might want to handle, we should instead allow specifying macro productions globally.

what do you mean?
Do you mean to group all the macro configuration options into "Macros" field, still containing one field for each kind of macro? Or do you have something else in mind?

I mean that we can configure macros in the format style, like "define A(X) class X {". I'm not 100% sure whether we would just try to use the Preprocessor for this, or whether we'd want to only allow a small subset of actual macros, but the general idea would be the same: The UnwrappedLineParser would parse the macro at the expansion location A(X) into an unwrapped line, and then parse the expansion into a child line, with the tokens tha tare not in the argument of the call being marked as fixed (parent child might also be better inverted).

That will allow folks to actually specify the semantics they care about instead of us growing ever increasing special-case logic for different types of macros.

Typz added a comment.Dec 1 2017, 6:00 AM

Definitely that would be much more expressive. But this approach is also incomplete: in case of namespace (and possibly others?), we also need to perform the reverse operation, e.g. to "generate" a macro call for rewriting the closing comment.

On top of this, I think that is really not trivial, I would prefer to move forward with these "simpler" patch, and change the whole macro configurations part in later commits...

klimek added a comment.Dec 1 2017, 6:35 AM
In D37813#941987, @Typz wrote:

Definitely that would be much more expressive. But this approach is also incomplete: in case of namespace (and possibly others?), we also need to perform the reverse operation, e.g. to "generate" a macro call for rewriting the closing comment.

On top of this, I think that is really not trivial, I would prefer to move forward with these "simpler" patch, and change the whole macro configurations part in later commits...

Looking at the tests, I see what you mean, but I'd actually vote to not add non-namespace closing comments, or at least make closing comments configurable in a different way - it seems very much unrelated?

Typz added a comment.Dec 1 2017, 7:20 AM

As far as "parsing" and formatting inside the block is concerned, this is indeed unrelated (and would totally work if all macros where specified with some preprocessor definitions).

But identifying the 'opening' token and generating the matching 'closing' comment are totally related; it would seem very strange to have an option to specify that TESTSUITE() macros are parsed as namespace, then another option to indicate that namespace declared by this macros are actually closed with another macro call...

klimek added a comment.Dec 1 2017, 7:55 AM
In D37813#942137, @Typz wrote:

As far as "parsing" and formatting inside the block is concerned, this is indeed unrelated (and would totally work if all macros where specified with some preprocessor definitions).

But identifying the 'opening' token and generating the matching 'closing' comment are totally related; it would seem very strange to have an option to specify that TESTSUITE() macros are parsed as namespace, then another option to indicate that namespace declared by this macros are actually closed with another macro call...

Putting the namespace into the macro gives the macro an abstraction - if somebody were to change, for example, the namespace portion with a class definition, would you still want the closing comments or not?

Typz added a comment.Dec 5 2017, 8:20 AM

I don't think this is really relevant for this tool: if someone changes the implementation of the macro, then *they* must indeed if it should not be formatted like a namespace (and keep the clang-format configuration unchanged), or if it should now be formatted like a class (and thus make changes to clang-format configuration). Here we are not defining what the macro does, but how clang-format should indent it : in most case I don't think the indentation format should actually depend on the way it is implemented...

klimek added a comment.Dec 6 2017, 2:31 AM
In D37813#945125, @Typz wrote:

I don't think this is really relevant for this tool: if someone changes the implementation of the macro, then *they* must indeed if it should not be formatted like a namespace (and keep the clang-format configuration unchanged), or if it should now be formatted like a class (and thus make changes to clang-format configuration). Here we are not defining what the macro does, but how clang-format should indent it : in most case I don't think the indentation format should actually depend on the way it is implemented...

Ok, that's probably where our different opinions come from - I would want a macro to be formatted to reflect how it's implemented, because otherwise I'm going to be surprised when I look at the implementation, and I consider surprises to be something to avoid in programming in general, where possible.

That said, I'm also then a bit confused by your tests - they seem to currently format a mixture of namespace and class formatting, and combine the indentation of a class-scope with namespace end comments.

Typz added a comment.Dec 6 2017, 7:36 AM

Ok, that's probably where our different opinions come from - I would want a macro to be formatted to reflect how it's implemented, because otherwise I'm going to be surprised when I look at the implementation, and I consider surprises to be something to avoid in programming in general, where possible.

As "author of the coding rules" (and thus writer of clang-format config), I would agree with you.
But I think the tool itself should not decide to enforce this : maybe there are cases where the macro is "conceptually" a namespace, but should be indented as a class ; in this case I think the tool should quite dumb, and simply do what was asked: if asked to format the macro as a namespace, it should do it, whatever the implementation is.

That said, on top of this "user choice" the tool could be smart, and automatically handle all macros based on their actual implementation: if the macro is actually implemented with a namespace (and there is no "explicit" definition of how it should be handled), then indeed the tool could format the whole block as a namespace, and so on... But unfortunately that would imply parsing numerous #include to get the definitions, and is definitely not the goal of this patch: this patch is really about letting the "user" decide how to handle the macro.

That said, I'm also then a bit confused by your tests - they seem to currently format a mixture of namespace and class formatting, and combine the indentation of a class-scope with namespace end comments.

This should not be the case: the goal here is that the macro is handled exactly like a namespace.
The NamespaceEndCommentFixer tests indeed contain some indentation, but it is not significant and is there only because I copied existing test cases and followed the same "style": but these tests do not actually perform any indentation, they simply handle the "end comment".
Do you have any specific test which confuses you?

I've talked with Daniel and we both believe this patch is not the right way to go forward for clang-format. It is adding configuration mechanisms that we need to maintain going forward, without addressing a general problem; specifically for how to handle macros, I believe there is a clear better way to do it that is also fundamentally more useful than adding extra knobs for every problem we encounter. Finally, these changes do not address a fundamental bug, and workarounds exist, so there is no large benefit to do this now in a non-principled way rather than wait for a more principled fix. Overall, I do understand that this is a trade-off, but I hope you understand that we sometimes have to make decisions.

Typz added a comment.Dec 18 2017, 3:04 AM

OK.

So you mean a solution like the one discussed earlier would be the way to go?

I mean that we can configure macros in the format style, like "define A(X) class X {". I'm not 100% sure whether we would just try to use the Preprocessor for this, or whether we'd want to only allow a small subset of actual macros, but the general idea would be the same: The UnwrappedLineParser would parse the macro at the expansion location A(X) into an unwrapped line, and then parse the expansion into a child line, with the tokens tha tare not in the argument of the call being marked as fixed (parent child might also be better inverted).

(As a side-note, I want to stress out that we would actually need a 'reversible' description to support the namespace case, to allow generating the end-of-namespace comment)

Is there any work on that side, any timeline when this may be supported ?

In D37813#958116, @Typz wrote:

OK.

So you mean a solution like the one discussed earlier would be the way to go?

I mean that we can configure macros in the format style, like "define A(X) class X {". I'm not 100% sure whether we would just try to use the Preprocessor for this, or whether we'd want to only allow a small subset of actual macros, but the general idea would be the same: The UnwrappedLineParser would parse the macro at the expansion location A(X) into an unwrapped line, and then parse the expansion into a child line, with the tokens tha tare not in the argument of the call being marked as fixed (parent child might also be better inverted).

(As a side-note, I want to stress out that we would actually need a 'reversible' description to support the namespace case, to allow generating the end-of-namespace comment)

I believe this will be needed for various reasons. The plan is to make this similar to how we format #ifdef trees, for which clang-format already has full support.

Is there any work on that side, any timeline when this may be supported ?

Some initial design work has been done, and Krasimir said that he's interested. No timeline though :(

Typz updated this revision to Diff 132838.Feb 5 2018, 8:48 AM

rebase

Typz added a comment.Feb 28 2018, 7:06 AM

Some initial design work has been done, and Krasimir said that he's interested. No timeline though :(

any update or progress maybe?

Typz added a comment.May 16 2018, 2:53 AM

Some initial design work has been done, and Krasimir said that he's interested. No timeline though :(

@klimek , @krasimir : any progress on this new preprocessor-like way to configure macros ?

Typz updated this revision to Diff 158309.Jul 31 2018, 9:52 AM

Regeneration documentation

Typz added a comment.Aug 1 2018, 2:43 AM

Ping!

This has been stale for a while now. Last comment indicate this is not the right approach, but some prototyping has been done a more generic approach, with no timeline.
I understand this, but in the meantime the problem is still there, with no solution at the moment...

Can we move this forward in the mean time, possibly marking the API as 'beta', 'unstable' or another way of saying it is subject to change. It can be removed when there is a better solution.
Or maybe there is now a timeline for the better solution? or the prototype is available somewhere, with some documentation on limits/works to be done, so that work can continue from there?

klimek added a comment.Aug 1 2018, 3:15 AM

The problem here is that we have different opinions on how the formatting on namespace macros should behave in the first place- I think they should behave like namespaces, you want them to be formatted differently.
Given that you want full control over the formatting of those macros, and them not actually be formatted exactly like namespaces or classes, I think we need a more generic mechanism for you to express that.

Typz added a comment.EditedOct 3 2018, 8:39 AM

The problem here is that we have different opinions on how the formatting on namespace macros should behave in the first place- I think they should behave like namespaces, you want them to be formatted differently.
Given that you want full control over the formatting of those macros, and them not actually be formatted exactly like namespaces or classes, I think we need a more generic mechanism for you to express that.

Not sure what you mean here. I want them to behave like namespaces as well, this is actually the use case I have... As implemented, they indeed behave exactly like namespaces :

TESTSUITE(a) {                       namespace a {
} // TESTSUITE(a)                    } // namespace a
                                VS
TESTSUITE(a) { TESTSUITE(b) {        namespace a { namespace b {
} // TESTSUITE(a::b)                 }} // namespace a::b

(as long as there is a single argument. When multiple arguments are used, I add to choose a heuristic...)

As far as I understand, the divergence is that you would want something to "match" the implementation of the macro, while I propose a simpler heuristic, which should work fine for namespaces...

Or am I missing something entirely?

klimek added a comment.Oct 4 2018, 2:35 AM

The problem here is that we have different opinions on how the formatting on namespace macros should behave in the first place- I think they should behave like namespaces, you want them to be formatted differently.
Given that you want full control over the formatting of those macros, and them not actually be formatted exactly like namespaces or classes, I think we need a more generic mechanism for you to express that.

Not sure what you mean here. I want them to behave like namespaces as well, this is actually the use case I have... As implemented, they indeed behave exactly like namespaces :

TESTSUITE(a) {                       namespace a {
} // TESTSUITE(a)                    } // namespace a
                                VS
TESTSUITE(a) { TESTSUITE(b) {        namespace a { namespace b {
} // TESTSUITE(a::b)                 }} // namespace a::b

I thought we had the discussion upthread that they indent differently from namespaces. I'm working on a patch that allows you to configure macros.

Typz added a comment.Oct 4 2018, 2:57 AM

The problem here is that we have different opinions on how the formatting on namespace macros should behave in the first place- I think they should behave like namespaces, you want them to be formatted differently.
Given that you want full control over the formatting of those macros, and them not actually be formatted exactly like namespaces or classes, I think we need a more generic mechanism for you to express that.

Not sure what you mean here. I want them to behave like namespaces as well, this is actually the use case I have... As implemented, they indeed behave exactly like namespaces :

TESTSUITE(a) {                       namespace a {
} // TESTSUITE(a)                    } // namespace a
                                VS
TESTSUITE(a) { TESTSUITE(b) {        namespace a { namespace b {
} // TESTSUITE(a::b)                 }} // namespace a::b

I thought we had the discussion upthread that they indent differently from namespaces. I'm working on a patch that allows you to configure macros.

The current behavior is that they indent differently from namespace, because clang does not know these macros are conceptually equivalent to namespaces. And this patch actually fixes that, letting clang know they are actually macros.

Herald added a project: Restricted Project. · View Herald TranscriptMay 17 2019, 8:04 AM

The problem here is that we have different opinions on how the formatting on namespace macros should behave in the first place- I think they should behave like namespaces, you want them to be formatted differently.
Given that you want full control over the formatting of those macros, and them not actually be formatted exactly like namespaces or classes, I think we need a more generic mechanism for you to express that.

Not sure what you mean here. I want them to behave like namespaces as well, this is actually the use case I have... As implemented, they indeed behave exactly like namespaces :

TESTSUITE(a) {                       namespace a {
} // TESTSUITE(a)                    } // namespace a
                                VS
TESTSUITE(a) { TESTSUITE(b) {        namespace a { namespace b {
} // TESTSUITE(a::b)                 }} // namespace a::b

I thought we had the discussion upthread that they indent differently from namespaces. I'm working on a patch that allows you to configure macros.

The current behavior is that they indent differently from namespace, because clang does not know these macros are conceptually equivalent to namespaces. And this patch actually fixes that, letting clang know they are actually macros.

You say "and this patch fixes that", but in your test cases it looks like the indent of things within the namespace is not like the indent for namespaces. I'm thoroughly confused.

Typz added a comment.Wed, May 22, 9:27 AM

The patch goal is indeed to indent the content of namespace-macro blocks like the content of any 'regular' namespace. So it should look like the content of a namespace, possibly depending on the choose style options. To sumarize, here is a detailed summary of the observable differences between macros vs normal namespace.

Without this patch, clang-format does not understand the macro call actually defines a namespace, thus:

  • the content of the namespace-macro block is indented, even when Style.NamespaceIndentation = FormatStyle::NI_None
TESTSUITE(A) {
  int i;                  // <--- should not be indented
}
namespace B {
int j;
}
  • similarly for nested namespaces, when Style.NamespaceIndentation = FormatStyle::NI_Inner :
TESTSUITE(A) {
TESTSUITE(B) {
    int i;                // <--- should be indented 2-chars only
}
}
namespace C {
namespace D {
  int j;
}
}
  • There is no automatic fix of namespace end comment when Style.FixNamespaceComments = true
TESTSUITE(A) {
  int i;
}                         // <--- should have a namespace end comment
namespace B {
  int j;
} // namespace B
  • Multiple nested namespaces are not compacted when Style.CompactNamespaces = true
TESTSUITE(A) {
TESTSUITE(B) {             //<--- should be merged with previous line
    int i;
}
}                         // <--- should be merged with previous line (and have a "combined" namespace end comment)
namespace A { namespace B {
int j;
} // namespace A::B

This patch fixes all these points, which hopefully leads to the exact same formatting when using the namespace keyword or the namespace-macros:

// Style.NamespaceIndentation = FormatStyle::NI_None
TESTSUITE(A) {
int i;
}
namespace B {
int j;
}

// Style.NamespaceIndentation = FormatStyle::NI_Inner
TESTSUITE(A) {
TESTSUITE(B) {
  int i;
}
}
namespace C {
namespace D {
  int j;
}
}

// Style.FixNamespaceComments = true
TESTSUITE(A) {
  int i;
} // TESTSUITE(A)
namespace B {
  int j;
} // namespace B

// Style.CompactNamespaces = true
TESTSUITE(A) { TESTSUITE(B) {
  int i;
}} // TESTSUITE(A::B)
namespace A { namespace B {
  int j;
}} // namespace A::B

I did not see any issue in my testing or reviewing the code, but if you see a place in the tests where it does not match this, please point it out !

The patch goal is indeed to indent the content of namespace-macro blocks like the content of any 'regular' namespace. So it should look like the content of a namespace, possibly depending on the choose style options. To sumarize, here is a detailed summary of the observable differences between macros vs normal namespace.

Without this patch, clang-format does not understand the macro call actually defines a namespace, thus:

  • the content of the namespace-macro block is indented, even when Style.NamespaceIndentation = FormatStyle::NI_None ` lang=cpp TESTSUITE(A) { int i; // <--- should not be indented } namespace B { int j; } `
  • similarly for nested namespaces, when Style.NamespaceIndentation = FormatStyle::NI_Inner : ` lang=cpp TESTSUITE(A) { TESTSUITE(B) { int i; // <--- should be indented 2-chars only } } namespace C { namespace D { int j; } } `
  • There is no automatic fix of namespace end comment when Style.FixNamespaceComments = true ` lang=cpp TESTSUITE(A) { int i; } <--- should have a namespace end comment namespace B { int j; } namespace B `
  • Multiple nested namespaces are not compacted when Style.CompactNamespaces = true ` lang=cpp TESTSUITE(A) { TESTSUITE(B) { <--- should be merged with previous line int i; } } <--- should be merged with previous line (and have a "combined" namespace end comment) namespace A { namespace B { int j; } // namespace A::B `

    ---

    This patch fixes all these points, which hopefully leads to the exact same formatting when using the namespace keyword or the namespace-macros: ` lang=cpp // Style.NamespaceIndentation = FormatStyle::NI_None TESTSUITE(A) { int i; } namespace B { int j; }

    // Style.NamespaceIndentation = FormatStyle::NI_Inner TESTSUITE(A) { TESTSUITE(B) { int i; } } namespace C { namespace D { int j; } }

    Style.FixNamespaceComments = true TESTSUITE(A) { int i; } TESTSUITE(A) namespace B { int j; } // namespace B

    Style.CompactNamespaces = true TESTSUITE(A) { TESTSUITE(B) { int i; }} TESTSUITE(A::B) namespace A { namespace B { int j; }} // namespace A::B `

    I did not see any issue in my testing or reviewing the code, but if you see a place in the tests where it does not match this, please point it out !

Ah, wow, I see what confused me now:
In the fixNamespaceEndComment tests you use an indent that clang-format would not produce. Can you please fix that, otherwise I find this super confusing :)

Typz added a comment.Thu, May 23, 2:52 AM

Ah, wow, I see what confused me now:
In the fixNamespaceEndComment tests you use an indent that clang-format would not produce. Can you please fix that, otherwise I find this super confusing :)

Can you point a specific exemple (in code) ?

There are many tests related to fixNamespaceEndComment(), which look fine to me and which in many cases are copied from similarly indented test cases with standard namespace's...
(Or was it an earlier problem, and I should fix all these existing test cases as well?)

klimek added inline comments.Thu, May 23, 4:23 AM
unittests/Format/NamespaceEndCommentsFixerTest.cpp
582 ↗(On Diff #158309)

All of the fixNamespaceEndComments tests are indented, but standard llvm style doesn't indent in namespaces at all iiuc.

Typz marked 2 inline comments as done.Fri, May 24, 9:23 AM
Typz added inline comments.
unittests/Format/NamespaceEndCommentsFixerTest.cpp
526 ↗(On Diff #158309)

Should I also fix these tests?

They already existed before this patch, but do not follow LLVM namespace indent style either.

582 ↗(On Diff #158309)

Ok, understood. I can fix that.

klimek added inline comments.Mon, May 27, 12:43 AM
unittests/Format/NamespaceEndCommentsFixerTest.cpp
526 ↗(On Diff #158309)

If you don't mind, that would be useful - doing it in a separate CL is fine, so we keep this one focused (and no review needed).

Typz updated this revision to Diff 201936.Wed, May 29, 8:40 AM
Typz marked an inline comment as done.
  • Rebase
  • Fix NamespaceEndCommentsFixerTest to match LLVM indent style
Typz marked 2 inline comments as done.Wed, May 29, 9:20 AM
Typz added inline comments.
unittests/Format/NamespaceEndCommentsFixerTest.cpp
526 ↗(On Diff #158309)

Done

This revision is now accepted and ready to land.Thu, Jun 6, 7:27 AM
Typz updated this revision to Diff 203425.Thu, Jun 6, 1:01 PM

Rebase

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptThu, Jun 6, 1:04 PM