[PATCH] D29401: Fix MSVC Compatibility around dependent type with missing 'typename'
erichkeane abandoned this revision. erichkeane added a comment. I don't really see a good solution for this, and haven't tried in a while, so I'll see if I can prep another version of this in the future. https://reviews.llvm.org/D29401 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29401: Fix MSVC Compatibility around dependent type with missing 'typename'
erichkeane added a comment. Well hmm... changing this from MSVC to always caused a ton of regressions. I no longer think that this is a proper patch as it sits. Additionally, it doesn't fix the A::TYPE *var3 condition. https://reviews.llvm.org/D29401 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29401: Fix MSVC Compatibility around dependent type with missing 'typename'
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:
> > 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.
> This is what I see for the testcase in your commit message on trunk:
>
> ```
> :7:4: error: expected ';' after expression
> S::TD varname =0;
>^
>;
> :7:14: error: use of undeclared identifier 'varname'
> S::TD varname =0;
> ^
> :7:3: error: missing 'typename' prior to dependent type name
> 'S::TD'
> S::TD varname =0;
> ^~
> :11:3: note: in instantiation of function template specialization
> 'foo' requested here
> foo();
> ^
> 3 errors generated.
> ```
>
> Technically speaking, we do get the "missing typename" message, but I still
> wouldn't call this result "a good error message".
Ah, I see! You're right, I got caught up in this a bunch. I'll look to see if
I can get both cases to be better. Thanks!
https://reviews.llvm.org/D29401
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29401: Fix MSVC Compatibility around dependent type with missing 'typename'
efriedma 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))) {
erichkeane wrote:
> 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.
This is what I see for the testcase in your commit message on trunk:
```
:7:4: error: expected ';' after expression
S::TD varname =0;
^
;
:7:14: error: use of undeclared identifier 'varname'
S::TD varname =0;
^
:7:3: error: missing 'typename' prior to dependent type name 'S::TD'
S::TD varname =0;
^~
:11:3: note: in instantiation of function template specialization
'foo' requested here
foo();
^
3 errors generated.
```
Technically speaking, we do get the "missing typename" message, but I still
wouldn't call this result "a good error message".
https://reviews.llvm.org/D29401
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29401: Fix MSVC Compatibility around dependent type with missing 'typename'
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::TYPE var2 = 2; // expected-warning {{missing 'typename' prior
to dependent type name}}
+A::TYPE var3 = 2; // expected-warning {{missing 'typename' prior to
dependent type name}}
+MissingTypename::A::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::TYPE const var3 = 2; // const after type
> A::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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29401: Fix MSVC Compatibility around dependent type with missing 'typename'
efriedma 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))) {
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.
Comment at: test/SemaCXX/MicrosoftCompatibility.cpp:222
+const A::TYPE var2 = 2; // expected-warning {{missing 'typename' prior
to dependent type name}}
+A::TYPE var3 = 2; // expected-warning {{missing 'typename' prior to
dependent type name}}
+MissingTypename::A::TYPE var4 = 2; // expected-warning {{missing
'typename' prior to dependent type name}}
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::TYPE const var3 = 2; // const after type
A::TYPE *var3 = 2; // we can't tell this is invalid until the template is
instantiated
```
https://reviews.llvm.org/D29401
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29401: Fix MSVC Compatibility around dependent type with missing 'typename'
erichkeane added a comment. Thoughts? https://reviews.llvm.org/D29401 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29401: Fix MSVC Compatibility around dependent type with missing 'typename'
erichkeane added a comment.
Some quick line-comments here.
Comment at: lib/Parse/ParseStmt.cpp:186
// found.
-if (Next.isNot(tok::coloncolon)) {
+if (Next.isNot(tok::coloncolon) && (!getLangOpts().MSVCCompat ||
+Next.isNot(tok::less))) {
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!
Comment at: test/SemaCXX/MicrosoftCompatibility.cpp:222
+const A::TYPE var2 = 2; // expected-warning {{missing 'typename' prior
to dependent type name}}
+A::TYPE var3 = 2; // expected-warning {{missing 'typename' prior to
dependent type name}}
+MissingTypename::A::TYPE var4 = 2; // expected-warning {{missing
'typename' prior to dependent type name}}
This is the line that previously failed. Curiously, the one above and below
seemed to succeed without this change.
https://reviews.llvm.org/D29401
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29401: Fix MSVC Compatibility around dependent type with missing 'typename'
erichkeane created this revision.
As we know, MSVC is pretty relaxed when it comes to 'typename'. Clang so far
does a pretty darn good job of permitting the behavior, however there was 1
missed case that compiles in MSVC (as of 2015), but not in clang. I've updated
the test for similar ones that also/still pass, but were not tested.
template
struct S {
typedef int TD;
};
template
void foo() {
S::TD varname =0;
}
void foo2(){
foo();
}
The above was previously an error in clang in -fms-compatibility mode. This
patch alters the parser in MSVC mode to correctly assume the 'typename' above
where necessary.
https://reviews.llvm.org/D29401
Files:
lib/Parse/ParseStmt.cpp
test/SemaCXX/MicrosoftCompatibility.cpp
Index: lib/Parse/ParseStmt.cpp
===
--- lib/Parse/ParseStmt.cpp
+++ lib/Parse/ParseStmt.cpp
@@ -183,7 +183,8 @@
// Look up the identifier, and typo-correct it to a keyword if it's not
// found.
-if (Next.isNot(tok::coloncolon)) {
+if (Next.isNot(tok::coloncolon) && (!getLangOpts().MSVCCompat ||
+Next.isNot(tok::less))) {
// Try to limit which sets of keywords should be included in typo
// correction based on what the next token is.
if (TryAnnotateName(/*IsAddressOfOperand*/ false,
Index: test/SemaCXX/MicrosoftCompatibility.cpp
===
--- test/SemaCXX/MicrosoftCompatibility.cpp
+++ test/SemaCXX/MicrosoftCompatibility.cpp
@@ -218,6 +218,9 @@
void function_missing_typename(const T::Type param)// expected-warning
{{missing 'typename' prior to dependent type name}}
{
const T::Type var = 2; // expected-warning {{missing 'typename' prior to
dependent type name}}
+const A::TYPE var2 = 2; // expected-warning {{missing 'typename' prior
to dependent type name}}
+A::TYPE var3 = 2; // expected-warning {{missing 'typename' prior to
dependent type name}}
+MissingTypename::A::TYPE var4 = 2; // expected-warning {{missing
'typename' prior to dependent type name}}
}
template void function_missing_typename(const D::Type param);
Index: lib/Parse/ParseStmt.cpp
===
--- lib/Parse/ParseStmt.cpp
+++ lib/Parse/ParseStmt.cpp
@@ -183,7 +183,8 @@
// Look up the identifier, and typo-correct it to a keyword if it's not
// found.
-if (Next.isNot(tok::coloncolon)) {
+if (Next.isNot(tok::coloncolon) && (!getLangOpts().MSVCCompat ||
+Next.isNot(tok::less))) {
// Try to limit which sets of keywords should be included in typo
// correction based on what the next token is.
if (TryAnnotateName(/*IsAddressOfOperand*/ false,
Index: test/SemaCXX/MicrosoftCompatibility.cpp
===
--- test/SemaCXX/MicrosoftCompatibility.cpp
+++ test/SemaCXX/MicrosoftCompatibility.cpp
@@ -218,6 +218,9 @@
void function_missing_typename(const T::Type param)// expected-warning {{missing 'typename' prior to dependent type name}}
{
const T::Type var = 2; // expected-warning {{missing 'typename' prior to dependent type name}}
+const A::TYPE var2 = 2; // expected-warning {{missing 'typename' prior to dependent type name}}
+A::TYPE var3 = 2; // expected-warning {{missing 'typename' prior to dependent type name}}
+MissingTypename::A::TYPE var4 = 2; // expected-warning {{missing 'typename' prior to dependent type name}}
}
template void function_missing_typename(const D::Type param);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
