[PATCH] D90448: [clang] Add type check for explicit instantiation of static data members

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.


================
Comment at: clang/include/clang/AST/ASTContext.h:2339-2353
   /// Determine whether the given types \p T1 and \p T2 are equivalent.
-  bool hasSameType(QualType T1, QualType T2) const {
-    return getCanonicalType(T1) == getCanonicalType(T2);
+  /// The lifetime qualifier of Objective C can be chosen to be ignored because
+  /// sometimes we don't want to take this into consideration.
+  bool hasSameType(QualType T1, QualType T2,
+                   bool IgnoreObjCLifetimeQual = false) const {
+    if (!IgnoreObjCLifetimeQual) {
+      return getCanonicalType(T1) == getCanonicalType(T2);
----------------
I'm guessing this function is pretty widely used - so this change might be a bit invasive/may not be what all callers want?

@rsmith - this is getting a bit out of my wheelhouse and I'd rather like you to take a look when you've got a chance


================
Comment at: clang/lib/Sema/SemaTemplate.cpp:10099-10101
+    // be generated in that case. Objective C lifetime qualifiers will be
+    // automatically added by compiler with option -fobjc-arc so it should be
+    // ignored when comparing types.
----------------
The ObjC comment seems a bit out of place here - since the code here doesn't seem to mention them. Maybe skip that part here, and leave it to be described up in the hasSameType function?


================
Comment at: clang/test/SemaCXX/template-explicit-instant-type-mismatch.cpp:15-17
+template<typename T> T c; //expected-note {{variable template 'c' declared here}}
+
+template int c<B>; //expected-error {{type 'int' of explicit instantiation of 'c' does not match expected type 'B'}}
----------------
I assume this was already tested somewhere else, perhaps? (since your change was about static member variables of class templates, and didn't touch the existing code for variable templates) So probably doesn't need re-testing here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90448