JonasToth added a comment. Whats left from my personal todo is adjusting the documentation. I think then this check can work as linter and if you want as fixer as well, but this has to be enable explicitly.
I think that fixing still has more value than harm, e.g. in your IDE/editor. The most annyoing part is that `const` may be applied multiple times. But you can delete the superfluous consts and then you are done, the variable will not be considered anymore. Did I miss important things? ================ Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp:46 + +void ConstCorrectnessCheck::registerMatchers(MatchFinder *Finder) { + const auto ConstType = hasType(isConstQualified()); ---------------- aaron.ballman wrote: > JonasToth wrote: > > aaron.ballman wrote: > > > JonasToth wrote: > > > > aaron.ballman wrote: > > > > > Should this check only fire in C++? I'm sort of on the fence. It's a > > > > > C++ core guideline, so it stands to reason it should be disabled for > > > > > C code. But const-correctness is a thing in C too. WDYT? > > > > I do not know all the subtle differences between C and C++ here. > > > > Judging from this: > > > > https://stackoverflow.com/questions/5248571/is-there-const-in-c the > > > > current form would not be correct for C for pointers. > > > > The assumptions of this check and especially the transformations are > > > > done for C++. I dont see a reason why the value-semantic would not > > > > apply for both. > > > > > > > > Maybe there is a way to make the code compatible for both languages. > > > > The easiest solution would probably to not do the 'pointer-as-value' > > > > transformation. This is not that relevant as a feature anyway. I expect > > > > not nearly as much usage of this option as for the others. > > > > > > > > In the end of the day i would like to support both C and C++. Right now > > > > it is untested and especially the transformation might break the code. > > > > It should run on both languages though, as there is no language > > > > checking. > > > > I will add some real world C code-bases for the transformation testing > > > > and see what happens :) > > > > Judging from this: > > > > https://stackoverflow.com/questions/5248571/is-there-const-in-c the > > > > current form would not be correct for C for pointers. > > > > > > Sure, there may be additional changes needed to support the other > > > oddities of C. I was asking more at the high level. > > > > > > > In the end of the day i would like to support both C and C++. Right now > > > > it is untested and especially the transformation might break the code. > > > > > > Another option is for us to restrict the check in its current form to > > > just C++ and then expose a C check from it in a follow-up (likely under a > > > different module than cppcodeguidelines). For instance, CERT has a > > > recommendation (not a rule) about this for C > > > (https://wiki.sei.cmu.edu/confluence/display/c/DCL00-C.+Const-qualify+immutable+objects) > > > and I would not be surprised to find it in other coding standards as > > > well. > > Another const check is probably better. The clang-tidy part should be > > minimal effort. I think there isn't alot of duplication either. > > We could still think of proper configurability of this check and alias it > > into other modules with proper defaults for C. > Okay, I'm sold -- can you not register the check unless in C++ mode? We can > add a C-specific check later. Register only for C++ ================ Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.h:31 + WarnPointersAsValues(Options.get("WarnPointersAsValues", 0)), + TransformValues(Options.get("TransformValues", 1)), + TransformReferences(Options.get("TransformReferences", 1)), ---------------- Deactivating the transformations by default, as they are not ready for production yet. ================ Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp:830 + +void complex_usage() { + int np_local0 = 42; ---------------- aaron.ballman wrote: > Here's another terrible one to try -- can you make sure we don't try to make > something const when the variable is evaluated in a context where it's > usually not: > ``` > int N = 1; // Can't make N 'const' because VLAs make everything awful > sizeof(int[++N]); > ``` Already covered ;) I was afraid :D Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54943/new/ https://reviews.llvm.org/D54943 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits