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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
