Page MenuHomePhabricator

[clang-format] Add Left/Right Const fixer capability
ClosedPublic

Authored by MyDeveloperDay on Nov 3 2019, 7:52 AM.

Details

Summary

Developers these days seem to argue over east vs west const like they used to argue over tabs vs whitespace or the various bracing style. These previous arguments were mainly eliminated with tools like clang-format that allowed those rules to become part of your style guide. Anyone who has been using clang-format in a large team over the last couple of years knows that we don't have those religious arguments any more, and code reviews are more productive.

https://www.youtube.com/watch?v=fv--IKZFVO8
https://mariusbancila.ro/blog/2018/11/23/join-the-east-const-revolution/
https://www.youtube.com/watch?v=z6s6bacI424

The purpose of this revision is to try to do the same for the East/West const discussion. Move the debate into the style guide and leave it there!

In addition to the new ConstStyle: Right or ConstStyle: Left there is an additional command-line argument --const-style=left/right which would allow an individual developer to switch the source back and forth to their own style for editing, and back to the committed style before commit. (you could imagine an IDE might offer such a switch)

The revision works by implementing a separate pass of the Annotated lines much like the SortIncludes and then create replacements for constant type declarations.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

FYI - there's nothing "anti-inclusive" about East/West.

While I'm certain that there can be confusion around why terms are or are not troublesome to some people and there needs to be space for those discussions, I'm also certain that telling someone that there is "nothing" to their concerns is not a good way to reach an understanding. Let's please be a bit more respectful when discussing concerns about using inclusive terminology (https://llvm.org/docs/CodeOfConduct.html#when-we-disagree-try-to-understand-why).

I think the relevance of Left/Right East/West as a setting is less important, as its more about an ordering, allowing some tokens to go to the Left and some to the Right of the "type"

QualifierAlignment: Custom
QualifierOrder: [ inline, static, type, const, volatile, restrict ]

While I currently include the Left/Right for some form of completeness I sort of feel i should kill them before we landing this, as I think even my defaults are a little subjective. (unless we can get concencus), hence I don't feel the need to bring back up the Left/Right/East/West debate. If its still a problem later we can add them.

if (Style.QualifierAlignment == FormatStyle::QAS_Right) {
      Style.QualifierOrder.clear();
      Style.QualifierOrder.push_back("type");
      Style.QualifierOrder.push_back("const");
      Style.QualifierOrder.push_back("volatile");
    } else if (Style.QualifierAlignment == FormatStyle::QAS_Left) {
      Style.QualifierOrder.clear();
      Style.QualifierOrder.push_back("const");
      Style.QualifierOrder.push_back("volatile");
      Style.QualifierOrder.push_back("type");
    }

QualifierAlignmentFixer need to process all the Left/Right passes internally before return the fixes on the original code. So now QualifierAlignmentFixer has its own inner set of passes which will be processed, and then "no op" fixes (replacements which make no change to the original code are removed (this was a problem before, as multiple passes causes changes that changed one way then changed it back "static const int a;" -> "const static int a;" -> "static const a;"

Move QualifierAlignmentFixer tests out to its own unit test file so Ultimately I can add more "Unit tests" on the functions

Still WIP but getting much closer.

I think the relevance of Left/Right East/West as a setting is less important, as its more about an ordering, allowing some tokens to go to the Left and some to the Right of the "type"

QualifierAlignment: Custom
QualifierOrder: [ inline, static, type, const, volatile, restrict ]

While I currently include the Left/Right for some form of completeness I sort of feel i should kill them before we landing this, as I think even my defaults are a little subjective. (unless we can get concencus), hence I don't feel the need to bring back up the Left/Right/East/West debate. If its still a problem later we can add them.

+1, I think this style of configuration is easier to reason about than left/right, east/west, before/after, etc because it's an explicit ordering.

Allow more QualifierFixer functions to be directly tested, remove some older commented code.

MyDeveloperDay marked 3 inline comments as done.Sep 14 2021, 1:29 AM
MyDeveloperDay added inline comments.
clang/docs/ClangFormatStyleOptions.rst
2086

If its ok I think we can stick with Qualifier although it does support static/inline/const/volatile/restrcit/constexpr I think the primary usage will be type const and const type

MyDeveloperDay marked an inline comment as done.
  • Resolve the final issue I had regarding overlapping replacements (was adding double spaces between keywords),
  • Add ability to add a warning into the documentation for modifying code. (as agreed via the RFC)

This now needs a reviewer and if we want to proceed someone has to be brave enough to accept it ;-)

Missed dump_format_style.py update

Remove debug code
Tidy a few comments
Remove Qualifier Order defaults (must be specified for Custom)

Really nice!

No attributes in there, do you think it would be difficult to add them? We can definitely do that in another change to move this one forward.

clang/include/clang/Format/Format.h
1863–1864

I would drop that use with caution. I think without the warning is big enough, and with it's too much.

clang/lib/Format/Format.cpp
2927

Nit: Unrelated (and unnecessary) change.

clang/lib/Format/QualifierAlignmentFixer.cpp
29

Out of curiosity, on what premise do you choose a static member function that is only used in this file or a local free function? I would always choose the latter (with an anon namespace), to keep the header smaller.

58

Does not need to be addressed here, but does LLVM have a string builder or so? This produces a lot of (de-)allocations.

130

I would prefer to match the order of functions in the header with the order in the cpp.

162

This name is legacy from just const volatile formatting?

348

Only else and assert? It does nothing if Alignment is something different, or?

367

Couldn't you just

LeftOrder.assign(std::reverse_iterator(type) /*maybe with a next or prev, haven't thought too much about it*/, Order.rend());
RightOrder.assign(std::next(type), Order.end());

?

clang/lib/Format/QualifierAlignmentFixer.h
25

I don't know what the LLVM style is on that, but I prefer using anytime over typedef.

31

Has the 8 some meaning? Then maybe give it a name. Or is it just coincidence that you repeat the 8 for QualifierTokens?

clang/unittests/Format/FormatTest.cpp
18233

Unused?

MyDeveloperDay marked 11 inline comments as done.

Address review comments.

  • reorder functions to match header

Fix an overlapping replacement issue when going from Right to Left (just jump past the token once done)

Allow the use of "TypenameMacros" to allow for Capitialized types (be aware they should be non pointer types and don't have to be macros, so may need its own future "NonPtrTypes" setting)

MyDeveloperDay added inline comments.Sep 21 2021, 1:38 AM
clang/lib/Format/QualifierAlignmentFixer.cpp
29

I've been pulled up on using anon namespaces before in previous reviews, personally I always use them myself in my code. To be honest I've been moving most of these functions into actual class statics functions so I can test the functions explicitly in the unit tests (anon namespaces aren't good for that obvs)

I'd like to leave them as statics so I can more easily do that move and add more tests if I find issues.

58

I'm not a massive fan of this either, but I'm unsure if LLVM has anything, I think getting functional first is important, we can go for fast if someone can point out a better pattern.

130

I can spin that around, (its going to mess with review comments, but I know what you mean)

348

You know this was my bad because I reused the alignment type because it had the left and right but this could be a boolean as its different from the QualifierAlignment, let me change this for clarity

367

There is probably a better way but it needs to handle "type" being the first or last element, I tried and it didn't quite work.

clang/lib/Format/QualifierAlignmentFixer.h
25

I was actually just following what is in Format.cpp

31

For SmallVector this is basically a "ShortStringOptimization" for vector i.e. its stack allocated until the vector goes over 8, I have 7 qualifiers, and I wanted it an order of 2 so hence 8 (its shouldn't grow (and heap allocation) unless we define more qualifier types (this only supports what I say for now)

I think the use of a literal is quite common in this case, its really just a hint, I think its ok to use without it being a variable.

If we don't specify types in the QualifierOrder (say static or inline as in default Left/Right) then don't push const through them just because they can be a type of qualifier we support.

i.e. only push through what we specify, ultimately this helps reduce the number of changes (caused by inline/static) being in different orders (perhaps unintentional) but where you don't care (i.e. you haven't specified them)

Its no longer essential to compare the fixes against the original code, as this was needed because of an artifact with adding double spaces and was resolved with checking for if the previous token already ended or started with a space, (this speeds things up a little).

Added a unit test to validate that const splitting unsigned and char correctly resolves.

clang/lib/Format/QualifierAlignmentFixer.cpp
58

Full support for that.

clang/lib/Format/QualifierAlignmentFixer.h
31

I know what it is. So the 8 is NumberOfSupportedQualifiers? My point is that if we add something, let's say attributes ;), one only need to change that constant and both vectors grow accordingly, so that no heap allocation happens.

Nothing I would block that change for.

clang/unittests/Format/QualifierFixerTest.cpp
574

This one would suffice, since we add the values within the test now. But actually I would go for Style.QualifierOrder = {"inline",...}; and no EXPECTs needed.

856

This is not correct, the documentation of TypenameMacros says

These are expected to be macros of the form:

STACK_OF(...)

and HRESULT is just a typedef.
So a test against STACK_OF(int) is useful. But the macro detection should be reworked, or maybe dropped?
The handling of LLVM_NODISARD basically boils down to handling attributes and AttributeMacros.

IdanHo added a subscriber: IdanHo.Sep 22 2021, 10:52 AM
MyDeveloperDay marked 3 inline comments as done.

Remove use of Typenamemacros (we'll solve this a different way later)

Use better vector initialization

clang/lib/Format/QualifierAlignmentFixer.cpp
450

I think this is still right, because it is a type (according to the configuration).

clang/lib/Format/QualifierAlignmentFixer.h
17

This clang-tidy isn't addressed.

clang/unittests/Format/QualifierFixerTest.cpp
123

In addition to clang-tidies message: It is unnecessary.

MyDeveloperDay marked 3 inline comments as done.

Fix clang-format and clang-tidy issues

clang/lib/Format/QualifierAlignmentFixer.cpp
450

yeah, let's do this a different way after we have landed this, I was thinking about having a list of "Types" that users could specify that would help us classify tok::identifiers a little better (could help with some of the casting issues we've seen before)

I notice in projects like Linux they make heavy use of these ForeachMacros and StatementMacros, I don't want to steal another list I'd rather we had one that was very specific to types, I think this could help

Let's give it a first shot. When it has landed maybe I find the time to look into the attributes. ;)

clang/lib/Format/QualifierAlignmentFixer.cpp
450

We can do that.

This revision is now accepted and ready to land.Sep 23 2021, 11:23 AM

If there are no other objections I'm going to land this shortly.

This revision was landed with ongoing or failed builds.Sep 23 2021, 12:01 PM
This revision was automatically updated to reflect the committed changes.

This broke buildbots that have -Werror specified. I pushed in a fix in https://reviews.llvm.org/rG76d845cb169f048cb6f2176c3e7a6534dc5af097.
Also, please consider formatting your commit messages to prevent wrapping.

This broke buildbots that have -Werror specified. I pushed in a fix in https://reviews.llvm.org/rG76d845cb169f048cb6f2176c3e7a6534dc5af097.
Also, please consider formatting your commit messages to prevent wrapping.

Ok perfect, thank you and thanks for the advice. I'll try turning on -Werror in my local builds

simon.giesecke added inline comments.
clang/docs/ClangFormatStyleOptions.rst
3233

This is pretty unclear, for a number of reasons:

  • First, this probably only refers to a setting other than QAS_Leave?
  • Second, "lead to incorrect code generation" seems to skip a step. In the first place, this seems to imply that a setting other than QAS_Leave might change the semantics of the source code.
  • Third, it's not clear to me when this would happen, and how likely that is. Does this mean "Non-default settings are experimental, and you shouldn't use this in production?" or rather "Under rare circumstances that are unlikely to happen in real code, a non-default setting might change semantics." At the minimum, could this give some example(s) when this happens?
clang/docs/ClangFormatStyleOptions.rst
3233
  • Yes.
  • Yes, it might change the semantics, that was the content of a huge discussion here in the review and on the mailing list. Consensus was found that non whitespace changes are acceptable with a big warning and off by default.
  • The latter, if we would have an example at hand in which cases it wouldn't work, we would fix that and not give it as an example. So to the best of our knowledge it does not break anything.

The warning text however might be improved.

MyDeveloperDay marked 2 inline comments as done.Sep 29 2021, 3:06 PM
MyDeveloperDay added inline comments.
clang/docs/ClangFormatStyleOptions.rst
3233

Agreed its tough to give a meaningful example that actually currently breaks code, but the example I was given was

#define INTPTR int *

const INTPTR a;

In this sense moving const changes this from

const int * a;

to

int * const a;

For this we overcome this by NOT supporting "UPPERCASE" identifiers incase they are macros, we have plans to add support for identifying "type identifiers"

I guess if someone does

#define intptr int *

Then this could go still go wrong, however I like to think that these examples are on the "edge" of good code. (should we pander to what could be considered bad style in the first place?)

The warning was a concession to those who felt its should be highlighted as an option that could change behaviour (like SortIncludes was famously identified), and as @HazardyKnusperkeks say, we are looking for people to tell us where this breaks so we can try and fix it. (macros are already an issue for clang-format the solution for which is clang-format off/on)

I personally feel the no need to elaborate further on the warning at this time, but I'm happy to support someone if they feel they want to extend it.

If you are concerned my advice is don't use this option. But clang-format can be used in an advisor capacity (with -n or --dry-run) and this can be used to notify cases of incorrect qualifier ordering without actually using it to change the code.

simon.giesecke added inline comments.Sep 30 2021, 1:00 AM
clang/docs/ClangFormatStyleOptions.rst
3233

Thanks a lot for the explanation. Sorry I hadn't read through the entire review discussion. I saw the comment in the generated documentation and then dug up this patch that introduced it.

I see how macros might break this (I think I ran into a similar problem when trying to fix the const alignment manually using some sed machinery, though luckily it broke the build and didn't end up in bad code generation). It would be good to understand if there are any cases not involving macros whose semantics are modified. Identifying macros via the naming convention can't be reliable. Even if one did that consistently in their own code base, chances are high that library headers don't follow the same conventions. And yes, probably cases where a qualifier is added via a macro is not good style anyway. I replaced that by std::add_const_t in the mentioned case. But the use cases of clang-format are probably not confined to code written in a good style.

I think it's good to extend the warning a bit, otherwise anyone reading it would need to dig up this patch and read through the review discussion to judge it. I'll leave a comment on D110801 as well.

If you are concerned my advice is don't use this option.

Well, I think if it works "reliably", it's very useful. I was searching for a way to harmonize the const/qualifier alignment style, and I first thought that would be clang-tidy land, and then found this clang-format patch. And I guess, maybe that's exactly the gap that leads to this problem: In clang-tidy, there would be the extra semantic information (we know what's a macro or type etc.) that would allow to prevent any semantic changes.