erichkeane added inline comments.

================
Comment at: clang/lib/Sema/SemaCXXScopeSpec.cpp:134
                                          "specifier in SFINAE context?");
             if (!hasReachableDefinition(PartialSpec))
               diagnoseMissingImport(SS.getLastQualifierNameLoc(), PartialSpec,
----------------
Fznamznon wrote:
> erichkeane wrote:
> > I would expect 'hasReachableDefinition' to check boht reachable AND 
> > definition :) 
> > 
> > That said, I doubt the 'missing import' error is an appropriate one here.
> > I would expect 'hasReachableDefinition' to check boht reachable AND 
> > definition :)
> 
> Well, it is implemented in a way that it actually expects definition to be 
> present. In some other places where 'hasReachableDefinition' is called there 
> is also a check that definition exists prior the call.
Hrmph, ok then.  Perhaps it is not the right place to change things anyway.


================
Comment at: clang/test/SemaCXX/undefined-partial-specialization.cpp:12
+template<typename T>
+void boo<T, true>::foo(){} // expected-error{{nested name specifier 'boo<T, 
true>::' for declaration does not refer into a class, class template or class 
template partial specialization}}
+
----------------
Fznamznon wrote:
> erichkeane wrote:
> > I don't think this is correct.  The diagnostic is inaccurate, it DOES refer 
> > to a class template partial specialization (I can see it on line 9!), but 
> > the problem is that it is incomplete.
> Huh, it seems it was my change that made this diagnostic inaccurate. I was 
> under impression that it shouldn't have done this.
> For c++17 it used to say "error: out-of-line definition of 'foo' from class 
> 'boo<type-parameter-0-0, true>' without definition" without crash. Now it is 
> inaccurate for both c++17 and c++20. I'll look into this more.
Yes, I would definitely expect the 'out-of-line-definition' diagnostic, like we 
do for non-partial specs, and the C++17 behavior.  Interesting that we've 
messed this up for C++20, should be fun to track down/figure out!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148330

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to