[PATCH] D54395: [clang-tidy] implement utility-function to add 'const' to variables

Roman Lebedev via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Sun Mar 17 00:22:54 PDT 2019


lebedev.ri added a comment.

I think this looks good now, one nit about test coverage.
Did you run this through your buildbot, any issues?



================
Comment at: clang-tidy/utils/FixItHintUtils.cpp:35
+static bool isValueType(QualType QT) { return isValueType(QT.getTypePtr()); }
+static bool isArrayType(QualType QT) { return isa<ArrayType>(QT.getTypePtr()); }
+static bool isReferenceType(QualType QT) {
----------------
JonasToth wrote:
> aaron.ballman wrote:
> > This file is replicating a bunch of functionality that exists on the `Type*` already. Why do this rather than have the caller do `QT->isArrayType()` and skip this function entirely? (Same exists elsewhere here.)
> I had the issue that typedefs are resolved, but shouldn't. If I am not mistaken `QT->isArrayType()` would go to the canconical type.
> 
> ```
> using MyPtr = int*;
> MyPtr foo = nullptr;
> ```
> Is treated wrong in that case.
There is a test for that, right?


================
Comment at: unittests/clang-tidy/AddConstTest.cpp:733
+  StringRef T = "template <typename T> void f(T v) \n";
+  StringRef S = "{ T target = v; }";
+  auto Cat = [&T](StringRef S) { return (T + S).str(); };
----------------
JonasToth wrote:
> alexfh wrote:
> > It would be interesting to see test cases with multiple instantiations of the template the fix applies to.
> I added test for a template function with many instantiations, but there should not be a difference between the instantiations, because only the original code would be transformed, and there the 'how it looks' counts, so in this case it will be treated as a value.
> Did I misinterpret your question?
How about https://en.cppreference.com/w/cpp/language/function_template#Explicit_instantiation ?
I don't see any tests for that.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54395





More information about the cfe-commits mailing list