[PATCH] D146719: [Clang] Improve diagnostics when using a concept as template argument

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/Parse/Parser.cpp:1886-1889
+  case Sema::NC_Concept:
   case Sema::NC_VarTemplate:
   case Sema::NC_FunctionTemplate:
   case Sema::NC_UndeclaredTemplate: {
----------------
cor3ntin wrote:
> aaron.ballman wrote:
> > Would this change make sense, to validate that we're still consuming the token in these three cases? I think it was an assumption we made previously, but it's not clear to me if the code used to consume something other than `<` as well.
> I'm not sure. You'll notice that using a var template without template argument does not trigger the assert. That's because `ClassifyName` will return `NC_NonType` instead of `NC_VarTemplate` in that case. I don't think that makes sense either, but that require more investigation. I do think however all template names not followed by arguments should produce a TemplateIdAnnotation so the change is not not correct :)
> 
> If you insist, i can put the assert back, but I'm not sure the assert was testing the right thing.
I don't insist on adding the assert, it was more a matter of trying to figure out whether the changes to the non-concept cases have changed parsing behavior in some subtle way. My thinking is that an assert that fails (signaling we are no longer consuming a token when we previously would have) would be easier for us to debug than a mysterious parsing failure. But if that assert would be wrong to add, then it makes me wonder what the test case is for it because then we've changed parsing behavior.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146719