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 
I don't see any tests for that.

  rCTE Clang Tools Extra



cfe-commits mailing list

Reply via email to