Differential D37813
clang-format: better handle namespace macros Authored by Typz on Sep 13 2017, 9:27 AM.
Details Other macros are used to declare namespaces, and should thus be handled TESTSUITE(Foo) {
TEST(MyFirstTest) {
assert(0);
}
} // TESTSUITE(Foo)This patch deals with this cases by introducing a new option to specify
Diff Detail
Event TimelineComment Actions 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. Comment Actions what do you mean? Comment Actions Argh, very sorry for the delay in response. 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. Comment Actions 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... Comment Actions 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? Comment Actions 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... Comment Actions 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? Comment Actions 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... Comment Actions 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. Comment Actions
As "author of the coding rules" (and thus writer of clang-format config), I would agree with you. 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.
This should not be the case: the goal here is that the macro is handled exactly like a namespace. Comment Actions 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. Comment Actions OK. So you mean a solution like the one discussed earlier would be the way to go?
(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 ? Comment Actions 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.
Some initial design work has been done, and Krasimir said that he's interested. No timeline though :( Comment Actions
any update or progress maybe? Comment Actions 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. 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. Comment Actions 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. Comment Actions 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? Comment Actions 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. Comment Actions 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. Comment Actions 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. Comment Actions 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:
TESTSUITE(A) { int i; // <--- should not be indented } namespace B { int j; }
TESTSUITE(A) { TESTSUITE(B) { int i; // <--- should be indented 2-chars only } } namespace C { namespace D { int j; } }
TESTSUITE(A) { int i; } // <--- should have a namespace end comment namespace B { int j; } // namespace B
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 ! Comment Actions Ah, wow, I see what confused me now: Comment Actions
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...
| ||||||||||||||||||