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