[PATCH] D29401: Fix MSVC Compatibility around dependent type with missing 'typename'

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.


================
Comment at: lib/Parse/ParseStmt.cpp:186
     // found.
-    if (Next.isNot(tok::coloncolon)) {
+    if (Next.isNot(tok::coloncolon) && (!getLangOpts().MSVCCompat ||
+        Next.isNot(tok::less))) {
----------------
efriedma wrote:
> erichkeane wrote:
> > Clang-tidy created this layout here that I'm not thrilled with, if OK, I'd like to move the entirety of the 2nd component to the "&&" on its own line.  Additionally, if anyone has a better way to do this logic, I'm all ears!
> Why is this checking for MSVCCompat?  I think we want to detect constructs like your testcase in all modes so we can generate a good error message.
We get a good error message here ("typename missing") in normal mode.  The issue here is that the examples below work in MSVC's relaxed 'typename' situation, thus this should only be accepting code in MSVC mode, right?  Or am I missing something.


================
Comment at: test/SemaCXX/MicrosoftCompatibility.cpp:222
+    const A<T>::TYPE var2 = 2; // expected-warning {{missing 'typename' prior to dependent type name}}
+    A<T>::TYPE var3 = 2; // expected-warning {{missing 'typename' prior to dependent type name}}
+    MissingTypename::A<T>::TYPE var4 = 2; // expected-warning {{missing 'typename' prior to dependent type name}}
----------------
efriedma wrote:
> erichkeane wrote:
> > This is the line that previously failed.  Curiously, the one above and below seemed to succeed without this change.
> The first one is obviously a declaration because of the "const" keyword, so we don't follow the same codepath.  I would guess the last one hits the "Next.isNot(tok::coloncolon)" check in the if statment you're modifying.
> 
> A couple more related testcases:
> 
> ```
> A<T>::TYPE const var3 = 2; // const after type
> A<T>::TYPE *var3 = 2; // we can't tell this is invalid until the template is instantiated
> ```
Ah, right!  I'll add those tests as well!


https://reviews.llvm.org/D29401