[PATCH] D123808: [clang] Fix the bogus diagnostic introduced by the newly-added UsingTemplate Kind.

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:11014
     // Check that the return type is written as a specialization of
     // the template specified as the deduction-guide's name.
     ParsedType TrailingReturnType = Chunk.Fun.getTrailingReturnType();
----------------
I'd add here "The template name may not be qualified. [temp.deduct.guide]".

This is pretty important to the logic here and never explicitly written down.


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:11027
+      auto TKind = SpecifiedName.getKind();
+      // Template names within TemplateSpecializationType are never qualified
+      // template names!
----------------
It's not quite clear what you're implying by this:
 - we want to allow qualified names, but they're not QualifiedTemplateNames? (this was my first reading)
 - we want to disallow qualified names, but don't have to worry about QualifiedTemplateNames.

The latter is correct, because the grammar only allows a simple-template-id (the name must be an identifier, no NNS allowed).

I'm not actually sure we want a comment here about qualified names at all - not including it in the || is correct whether or not it could occur here.

By the way, the following code is correctly rejected by clang, but accepted by GCC and MSVC:

```
namespace ns {
template <typename> class X {};
template <typename T> X(T) -> ns::X<T>;
}
```

(We're in the right scope, so this isn't just about secondary diagnostics)


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:11029
+      // template names!
+      if ((TKind == TemplateName::Template ||
+           TKind == TemplateName::UsingTemplate) &&
----------------
The meaning of this || is not clear.

Maybe `bool SimplyWritten = ...`?

Maybe also `// A Using TemplateName can't actually be valid (either it's qualified, or we're in the wrong scope). But we have diagnosed these problems already.`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123808