[PATCH] D54943: [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Jonas Toth via Phabricator via cfe-commits]

JonasToth marked 2 inline comments as done.
JonasToth added inline comments.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-templates.cpp:16
+  int value_int = 42;
+  // CHECK-MESSAGES:[[@LINE-1]]:3: warning: variable 'value_int' of type 'int' can be declared 'const'
+}
----------------
LegalizeAdulthood wrote:
> `CHECK-FIXES`?
see my comment in the other test.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-pointer-as-values.cpp:12
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double *' can be declared 'const'
+  // CHECK-FIXES: const
+}
----------------
0x8000-0000 wrote:
> LegalizeAdulthood wrote:
> > Shouldn't this validate that the `const` was placed in the correct position?
> > e.g. `const double *` is a different meaning from `double *const`
> > 
> > Apply to all the other `CHECK-FIXES` as well
> Can we have the checker merged in first, then we can worry about the automatic fixer?
the checker is its own utility with its own tests and proper test coverage.
yes `const double*` and `double* const` are different and are correctly inserted, but that is not tested here, but here: https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp

Similar to the actual `const` analysis. That has its own test-suite (https://github.com/llvm/llvm-project/blob/main/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp)

The tests here are concerned with the diagnostics and "real" code. Afterall, this functionality is too complex to have all of it checked with these kind of tests.
I think the separate testing in specialized unit-tests (as is now) for the specialized functions is the right approach and the `CHECK-FIXES` are not necessary in this instance, maybe even bad, because it makes the tests unclearer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943