[PATCH] D131386: [clang-tidy] Added `ConstAlignment` option to `misc-const-correctness`

Mon Aug 15 11:11:21 PDT 2022

aaron.ballman added subscribers: owenpan, MyDeveloperDay.
aaron.ballman added a comment.

In D131386#3723490 <https://reviews.llvm.org/D131386#3723490>, @Mordante wrote:

> In D131386#3722749 <https://reviews.llvm.org/D131386#3722749>, @aaron.ballman wrote:
>
>> We leave formatting decisions in clang-tidy to clang-format and I don't think we should deviate from that policy here without a very clear understanding of when we should relax that restriction. That said, I'm personally not certain we should have such an option (the long-term goal has generally been to integrate clang-format functionality into clang-tidy so there can be an option to just run format after applying fixes in a TU). Is there a compelling reason we should have it?
>
> The reason for me to work on this feature is the fact that clang-format doesn't work reliable for changing the location of the `const`. After using clang-tidy and clang-format some `const`s were still one the right hand side. I didn't do a deep investigation, but it seems to affect classes in namespaces. So some occurrences of `std::string` were affected, but not all. So in the current state I can't use this clang-tidy fix.

CC @MyDeveloperDay and @owenpan for awareness.

> Since clang-format doesn't fully parse the code I wonder whether all cases can be properly fixed. On the other hand clang-tidy has all information available.

Mostly. clang-tidy will miss everything in a preprocessor conditional block, as an example of what it can't handle but clang-format can.

> Would integrating clang-format in clang-tidy mean clang-format will use the full information of the AST?

I wouldn't imagine that to be the case (that'd be a pretty big change to the architecture of clang-format; basically a rewrite). I would expect it to be integrated more as a library to call into to perform the post-fix formatting to just specific ranges of code. However, it is interesting to think about the fact that such a library could potentially accept an optional AST node representing the root of the AST represented by the text as a way to help it disambiguate situations it couldn't otherwise figure out.

In D131386#3723254 <https://reviews.llvm.org/D131386#3723254>, @carlosgalvezp wrote:

> FWIW, there are already other checks that have formatting settings, for example:
> https://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines/pro-bounds-constant-array-index.html#cmdoption-arg-includestyle

Those docs are pretty terrible because I have no idea what an "llvm-style" include even *is*.

IIRC, we added this because we added the include sorter to clang-tidy and at the time, it was deemed unsafe to add to clang-format because of the likelihood of breaking code. IMO, it's on the borderline as to whether we should have added it or not -- it's the same situation I was worried about above where we add a feature to clang-tidy and then clang-format later gets the ability to perform the formatting and so the two gradually drift out of sync. Then again, it seems our checks have drifted out of sync with what IncludeSorter supports because there's a third option for google Objective C formatting style (whatever that is).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131386/new/

https://reviews.llvm.org/D131386