[PATCH] D91129: Print source location in the error message when parens are missing around sizeof typename and the expression is inside macro expansion

[Shivanshu Goyal via Phabricator via cfe-commits]

shivanshu3 marked an inline comment as done.
shivanshu3 added inline comments.



Comment at: clang/lib/Parse/ParseExpr.cpp:2267-2280
+SourceLocation OpTokLoc = OpTok.getLocation();
+if (OpTokLoc.isMacroID()) {
+  SourceLocation OpTokExpansionLoc =
+  PP.getSourceManager().getFileLoc(OpTokLoc);
+  Diag(OpTokExpansionLoc,
+   diag::err_expected_parentheses_around_typename)
+  << OpTok.getName();

rsmith wrote:
> I don't think we should be assuming that `getLocForEndOfToken` will fail if 
> and only if the token is a macro -- that seems brittle. It would seem better 
> to check whether it actually failed and respond to that directly. Would 
> something like this suggestion work?
Yup, I like that better, thank you for the suggestion!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91129

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

[PATCH] D91129: Print source location in the error message when parens are missing around sizeof typename and the expression is inside macro expansion

[Shivanshu Goyal via Phabricator via cfe-commits]

shivanshu3 updated this revision to Diff 312055.
shivanshu3 added a comment.

Don't assume that `getLocForEndOfToken` will fail if and only if the token is a 
macro


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91129

Files:
  clang/lib/Parse/ParseExpr.cpp
  clang/test/Parser/sizeof-missing-parens.c


Index: clang/test/Parser/sizeof-missing-parens.c
===
--- /dev/null
+++ clang/test/Parser/sizeof-missing-parens.c
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+void Foo(int);
+
+#define Bar(x) Foo(x)
+
+void Baz() {
+  Foo(sizeof int); // expected-error {{expected parentheses around type name 
in sizeof expression}}
+  Bar(sizeof int); // expected-error {{expected parentheses around type name 
in sizeof expression}}
+}
Index: clang/lib/Parse/ParseExpr.cpp
===
--- clang/lib/Parse/ParseExpr.cpp
+++ clang/lib/Parse/ParseExpr.cpp
@@ -2266,10 +2266,15 @@
 
 SourceLocation LParenLoc = PP.getLocForEndOfToken(OpTok.getLocation());
 SourceLocation RParenLoc = PP.getLocForEndOfToken(PrevTokLocation);
-Diag(LParenLoc, diag::err_expected_parentheses_around_typename)
-  << OpTok.getName()
-  << FixItHint::CreateInsertion(LParenLoc, "(")
-  << FixItHint::CreateInsertion(RParenLoc, ")");
+if (LParenLoc.isInvalid() || RParenLoc.isInvalid()) {
+  Diag(OpTok.getLocation(),
+   diag::err_expected_parentheses_around_typename)
+  << OpTok.getName();
+} else {
+  Diag(LParenLoc, diag::err_expected_parentheses_around_typename)
+  << OpTok.getName() << FixItHint::CreateInsertion(LParenLoc, "(")
+  << FixItHint::CreateInsertion(RParenLoc, ")");
+}
 isCastExpr = true;
 return ExprEmpty();
   }


Index: clang/test/Parser/sizeof-missing-parens.c
===
--- /dev/null
+++ clang/test/Parser/sizeof-missing-parens.c
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+void Foo(int);
+
+#define Bar(x) Foo(x)
+
+void Baz() {
+  Foo(sizeof int); // expected-error {{expected parentheses around type name in sizeof expression}}
+  Bar(sizeof int); // expected-error {{expected parentheses around type name in sizeof expression}}
+}
Index: clang/lib/Parse/ParseExpr.cpp
===
--- clang/lib/Parse/ParseExpr.cpp
+++ clang/lib/Parse/ParseExpr.cpp
@@ -2266,10 +2266,15 @@
 
 SourceLocation LParenLoc = PP.getLocForEndOfToken(OpTok.getLocation());
 SourceLocation RParenLoc = PP.getLocForEndOfToken(PrevTokLocation);
-Diag(LParenLoc, diag::err_expected_parentheses_around_typename)
-  << OpTok.getName()
-  << FixItHint::CreateInsertion(LParenLoc, "(")
-  << FixItHint::CreateInsertion(RParenLoc, ")");
+if (LParenLoc.isInvalid() || RParenLoc.isInvalid()) {
+  Diag(OpTok.getLocation(),
+   diag::err_expected_parentheses_around_typename)
+  << OpTok.getName();
+} else {
+  Diag(LParenLoc, diag::err_expected_parentheses_around_typename)
+  << OpTok.getName() << FixItHint::CreateInsertion(LParenLoc, "(")
+  << FixItHint::CreateInsertion(RParenLoc, ")");
+}
 isCastExpr = true;
 return ExprEmpty();
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91659: Allow enum typedefs to be referenced with the 'enum' specifier under MSVC compat mode

[Shivanshu Goyal via Phabricator via cfe-commits]

shivanshu3 added a comment.

Ping!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91659

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

[PATCH] D91129: Print source location in the error message when parens are missing around sizeof typename and the expression is inside macro expansion

[Shivanshu Goyal via Phabricator via cfe-commits]

shivanshu3 added a comment.

Ping!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91129

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

[PATCH] D91129: Print source location in the error message when parens are missing around sizeof typename and the expression is inside macro expansion

[Shivanshu Goyal via Phabricator via cfe-commits]

shivanshu3 updated this revision to Diff 308827.
shivanshu3 added a comment.

Clang format the test file. Forgot to do it before.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91129

Files:
  clang/lib/Parse/ParseExpr.cpp
  clang/test/Parser/sizeof-missing-parens.c


Index: clang/test/Parser/sizeof-missing-parens.c
===
--- /dev/null
+++ clang/test/Parser/sizeof-missing-parens.c
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+void Foo(int);
+
+#define Bar(x) Foo(x)
+
+void Baz() {
+  Foo(sizeof int); // expected-error {{expected parentheses around type name 
in sizeof expression}}
+  Bar(sizeof int); // expected-error {{expected parentheses around type name 
in sizeof expression}}
+}
Index: clang/lib/Parse/ParseExpr.cpp
===
--- clang/lib/Parse/ParseExpr.cpp
+++ clang/lib/Parse/ParseExpr.cpp
@@ -2264,12 +2264,20 @@
 Declarator DeclaratorInfo(DS, DeclaratorContext::TypeName);
 ParseDeclarator(DeclaratorInfo);
 
-SourceLocation LParenLoc = PP.getLocForEndOfToken(OpTok.getLocation());
-SourceLocation RParenLoc = PP.getLocForEndOfToken(PrevTokLocation);
-Diag(LParenLoc, diag::err_expected_parentheses_around_typename)
-  << OpTok.getName()
-  << FixItHint::CreateInsertion(LParenLoc, "(")
-  << FixItHint::CreateInsertion(RParenLoc, ")");
+SourceLocation OpTokLoc = OpTok.getLocation();
+if (OpTokLoc.isMacroID()) {
+  SourceLocation OpTokExpansionLoc =
+  PP.getSourceManager().getFileLoc(OpTokLoc);
+  Diag(OpTokExpansionLoc,
+   diag::err_expected_parentheses_around_typename)
+  << OpTok.getName();
+} else {
+  SourceLocation LParenLoc = PP.getLocForEndOfToken(OpTokLoc);
+  SourceLocation RParenLoc = PP.getLocForEndOfToken(PrevTokLocation);
+  Diag(LParenLoc, diag::err_expected_parentheses_around_typename)
+  << OpTok.getName() << FixItHint::CreateInsertion(LParenLoc, "(")
+  << FixItHint::CreateInsertion(RParenLoc, ")");
+}
 isCastExpr = true;
 return ExprEmpty();
   }


Index: clang/test/Parser/sizeof-missing-parens.c
===
--- /dev/null
+++ clang/test/Parser/sizeof-missing-parens.c
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+void Foo(int);
+
+#define Bar(x) Foo(x)
+
+void Baz() {
+  Foo(sizeof int); // expected-error {{expected parentheses around type name in sizeof expression}}
+  Bar(sizeof int); // expected-error {{expected parentheses around type name in sizeof expression}}
+}
Index: clang/lib/Parse/ParseExpr.cpp
===
--- clang/lib/Parse/ParseExpr.cpp
+++ clang/lib/Parse/ParseExpr.cpp
@@ -2264,12 +2264,20 @@
 Declarator DeclaratorInfo(DS, DeclaratorContext::TypeName);
 ParseDeclarator(DeclaratorInfo);
 
-SourceLocation LParenLoc = PP.getLocForEndOfToken(OpTok.getLocation());
-SourceLocation RParenLoc = PP.getLocForEndOfToken(PrevTokLocation);
-Diag(LParenLoc, diag::err_expected_parentheses_around_typename)
-  << OpTok.getName()
-  << FixItHint::CreateInsertion(LParenLoc, "(")
-  << FixItHint::CreateInsertion(RParenLoc, ")");
+SourceLocation OpTokLoc = OpTok.getLocation();
+if (OpTokLoc.isMacroID()) {
+  SourceLocation OpTokExpansionLoc =
+  PP.getSourceManager().getFileLoc(OpTokLoc);
+  Diag(OpTokExpansionLoc,
+   diag::err_expected_parentheses_around_typename)
+  << OpTok.getName();
+} else {
+  SourceLocation LParenLoc = PP.getLocForEndOfToken(OpTokLoc);
+  SourceLocation RParenLoc = PP.getLocForEndOfToken(PrevTokLocation);
+  Diag(LParenLoc, diag::err_expected_parentheses_around_typename)
+  << OpTok.getName() << FixItHint::CreateInsertion(LParenLoc, "(")
+  << FixItHint::CreateInsertion(RParenLoc, ")");
+}
 isCastExpr = true;
 return ExprEmpty();
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91659: Allow enum typedefs to be referenced with the 'enum' specifier under MSVC compat mode

[Shivanshu Goyal via Phabricator via cfe-commits]

shivanshu3 added inline comments.



Comment at: clang/lib/Sema/SemaDecl.cpp:15752-15754
+  bool AnonymousEnumEligible = getLangOpts().MSVCCompat &&
+   (Kind == TTK_Enum) &&
+   Tag->getDeclName().isEmpty();

shivanshu3 wrote:
> rsmith wrote:
> > In the anonymous union case, we also need to check for qualifiers on the 
> > typedef type, and more broadly perhaps we should be using 
> > `getAnonDeclWithTypedefName` here too. Presumably the new case should also 
> > only apply for `TUK_Reference`.
> I added a check for TUK_Reference. But I don't fully understand your other 
> comment about checking the qualifiers on the typedef type. Could you please 
> explain a little more?
@rsmith Since my newest iteration removes the restriction for the enum to be 
anonymous, I'm guessing I can mark this comment as resolved?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91659

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

[PATCH] D91659: Allow enum typedefs to be referenced with the 'enum' specifier under MSVC compat mode

[Shivanshu Goyal via Phabricator via cfe-commits]

shivanshu3 updated this revision to Diff 308803.
shivanshu3 retitled this revision from "Allow anonymous enum typedefs to be 
referenced with the 'enum' specifier under MSVC compat mode" to "Allow enum 
typedefs to be referenced with the 'enum' specifier under MSVC compat mode".
shivanshu3 edited the summary of this revision.
shivanshu3 added a comment.

Previously for some reason I thought that MSVC only allowed this behavior for 
anonymous enums. But now I realize that the enum doesn't have to be anonymous. 
So the following code should be legal too with '-fms-compatibility':

  typedef enum FooEnum {
A
  } Foo;
  
  void func() {
enum Foo foo;
  }


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91659

Files:
  clang/lib/Sema/SemaDecl.cpp
  clang/test/Sema/enum-typedef-msvc.c
  clang/test/Sema/enum-typedef-msvc.cpp


Index: clang/test/Sema/enum-typedef-msvc.cpp
===
--- /dev/null
+++ clang/test/Sema/enum-typedef-msvc.cpp
@@ -0,0 +1,17 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -fms-compatibility %s
+
+typedef enum FooEnum {
+  A,
+} Foo;
+
+class AMyInterface {
+  virtual void MyFunc(enum Foo *param) = 0;
+};
+
+class MyImpl : public AMyInterface {
+  virtual void MyFunc(enum Foo *param) override {}
+};
+
+enum Foo myEnum;
+
+// expected-no-diagnostics
Index: clang/test/Sema/enum-typedef-msvc.c
===
--- /dev/null
+++ clang/test/Sema/enum-typedef-msvc.c
@@ -0,0 +1,22 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fsyntax-only -verify -fms-compatibility -DUSE_MSVC_COMPAT 
%s
+
+typedef enum FooEnum {
+  A
+} Foo;
+
+void func() {
+#ifdef USE_MSVC_COMPAT
+  enum Foo foo; // expected-no-diagnostics
+#else
+  enum Foo foo; // expected-error {{variable has incomplete type 'enum Foo'}} 
// expected-note {{forward declaration of 'enum Foo'}}
+#endif
+  (void)foo;
+}
+
+#ifdef USE_MSVC_COMPAT
+enum Foo foo2; // expected-no-diagnostics
+#else
+enum Foo foo2;  // expected-error {{tentative definition has type 'enum Foo' 
that is never completed}}
+// expected-note@-1 {{forward declaration of 'enum Foo'}}
+#endif
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -15539,6 +15539,21 @@
 // shouldn't be diagnosing.
 LookupName(Previous, S);
 
+// Under MSVC, the 'enum' specifier can be used for typedef'd enums.
+// Note that lookup only fails in C, not C++, so this if condition
+// is only used for C code.
+if (getLangOpts().MSVCCompat && Kind == TTK_Enum && Previous.empty() &&
+TUK == TUK_Reference) {
+  LookupResult TypedefEnumLookup(*this, Name, NameLoc, LookupOrdinaryName,
+ Redecl);
+  LookupName(TypedefEnumLookup, S);
+
+  if (auto *TD = TypedefEnumLookup.getAsSingle())
+if (auto *TT = TD->getUnderlyingType()->getAs())
+  if (auto *ED = dyn_cast_or_null(TT->getDecl()))
+Previous.addDecl(ED);
+}
+
 // When declaring or defining a tag, ignore ambiguities introduced
 // by types using'ed into this scope.
 if (Previous.isAmbiguous() &&
@@ -15721,9 +15736,12 @@
   if (TypedefNameDecl *TD = dyn_cast(PrevDecl)) {
 if (const TagType *TT = TD->getUnderlyingType()->getAs()) {
   TagDecl *Tag = TT->getDecl();
-  if (Tag->getDeclName() == Name &&
-  Tag->getDeclContext()->getRedeclContext()
-  ->Equals(TD->getDeclContext()->getRedeclContext())) {
+  bool AllowLookupUsingTypedefName = getLangOpts().MSVCCompat &&
+ TUK == TUK_Reference &&
+ Kind == TTK_Enum;
+  if ((Tag->getDeclName() == Name || AllowLookupUsingTypedefName) &&
+  Tag->getDeclContext()->getRedeclContext()->Equals(
+  TD->getDeclContext()->getRedeclContext())) {
 PrevDecl = Tag;
 Previous.clear();
 Previous.addDecl(Tag);


Index: clang/test/Sema/enum-typedef-msvc.cpp
===
--- /dev/null
+++ clang/test/Sema/enum-typedef-msvc.cpp
@@ -0,0 +1,17 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -fms-compatibility %s
+
+typedef enum FooEnum {
+  A,
+} Foo;
+
+class AMyInterface {
+  virtual void MyFunc(enum Foo *param) = 0;
+};
+
+class MyImpl : public AMyInterface {
+  virtual void MyFunc(enum Foo *param) override {}
+};
+
+enum Foo myEnum;
+
+// expected-no-diagnostics
Index: clang/test/Sema/enum-typedef-msvc.c
===
--- /dev/null
+++ clang/test/Sema/enum-typedef-msvc.c
@@ -0,0 +1,22 @@
+// RUN: %clang_cc1 -fsyntax-only 

[PATCH] D91659: Allow anonymous enum typedefs to be referenced with the 'enum' specifier under MSVC compat mode

[Shivanshu Goyal via Phabricator via cfe-commits]

shivanshu3 marked 3 inline comments as done.
shivanshu3 added a comment.

Thank you very much for the review @rsmith!




Comment at: clang/lib/Sema/SemaDecl.cpp:15752-15754
+  bool AnonymousEnumEligible = getLangOpts().MSVCCompat &&
+   (Kind == TTK_Enum) &&
+   Tag->getDeclName().isEmpty();

rsmith wrote:
> In the anonymous union case, we also need to check for qualifiers on the 
> typedef type, and more broadly perhaps we should be using 
> `getAnonDeclWithTypedefName` here too. Presumably the new case should also 
> only apply for `TUK_Reference`.
I added a check for TUK_Reference. But I don't fully understand your other 
comment about checking the qualifiers on the typedef type. Could you please 
explain a little more?



Comment at: clang/test/Sema/enum-typedef-msvc.c:15
+  (void)foo;
+}

rsmith wrote:
> Please also add a testcase for the situation where the use of `enum Foo` 
> occurs in the same scope as the typedef and anonymous `enum` declaration. 
> (MSVC has weird behavior here that we don't necessarily need to follow, but 
> we should make sure we at least do something defensible.)
Oh interesting! I didn't realize MSVC produces an error in that case. Clang 
does not produce an error with these changes in that case. Is that OK?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91659

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

[PATCH] D91659: Allow anonymous enum typedefs to be referenced with the 'enum' specifier under MSVC compat mode

[Shivanshu Goyal via Phabricator via cfe-commits]

shivanshu3 updated this revision to Diff 305971.
shivanshu3 added a reviewer: rsmith.
shivanshu3 added a comment.

Addressing some of rsmith's comments:

- Add a test case where an enum variable is declared in the same scope as the 
enum definition
- Fix a style issue with parentheses
- Simplify the if condition for the C case
- The C++ case should only apply for TUK_Reference


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91659

Files:
  clang/lib/Sema/SemaDecl.cpp
  clang/test/Sema/enum-typedef-msvc.c
  clang/test/Sema/enum-typedef-msvc.cpp


Index: clang/test/Sema/enum-typedef-msvc.cpp
===
--- /dev/null
+++ clang/test/Sema/enum-typedef-msvc.cpp
@@ -0,0 +1,18 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -fms-compatibility %s
+
+typedef enum {
+  First,
+  Second
+} MyEnum;
+
+class AMyInterface {
+  virtual void MyFunc(enum MyEnum *param) = 0;
+};
+
+class MyImpl : public AMyInterface {
+  virtual void MyFunc(enum MyEnum *param) override {}
+};
+
+enum MyEnum myEnum;
+
+// expected-no-diagnostics
Index: clang/test/Sema/enum-typedef-msvc.c
===
--- /dev/null
+++ clang/test/Sema/enum-typedef-msvc.c
@@ -0,0 +1,22 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fsyntax-only -verify -fms-compatibility -DUSE_MSVC_COMPAT 
%s
+
+typedef enum {
+  A
+} Foo;
+
+void func() {
+#ifdef USE_MSVC_COMPAT
+  enum Foo foo; // expected-no-diagnostics
+#else
+  enum Foo foo; // expected-error {{variable has incomplete type 'enum Foo'}} 
// expected-note {{forward declaration of 'enum Foo'}}
+#endif
+  (void)foo;
+}
+
+#ifdef USE_MSVC_COMPAT
+enum Foo foo2; // expected-no-diagnostics
+#else
+enum Foo foo2;  // expected-error {{tentative definition has type 'enum Foo' 
that is never completed}}
+// expected-note@-1 {{forward declaration of 'enum Foo'}}
+#endif
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -15539,6 +15539,21 @@
 // shouldn't be diagnosing.
 LookupName(Previous, S);
 
+// Under MSVC, the 'enum' specifier can be used for typedef'd enums.
+// Note that lookup only fails in C, not C++, so this if condition
+// is only used for C code.
+if (getLangOpts().MSVCCompat && Kind == TTK_Enum && Previous.empty() &&
+TUK == TUK_Reference) {
+  LookupResult TypedefEnumLookup(*this, Name, NameLoc, LookupOrdinaryName,
+ Redecl);
+  LookupName(TypedefEnumLookup, S);
+
+  if (auto *TD = TypedefEnumLookup.getAsSingle())
+if (auto *ED = dyn_cast_or_null(
+TD->getAnonDeclWithTypedefName(true)))
+  Previous.addDecl(ED);
+}
+
 // When declaring or defining a tag, ignore ambiguities introduced
 // by types using'ed into this scope.
 if (Previous.isAmbiguous() &&
@@ -15721,9 +15736,12 @@
   if (TypedefNameDecl *TD = dyn_cast(PrevDecl)) {
 if (const TagType *TT = TD->getUnderlyingType()->getAs()) {
   TagDecl *Tag = TT->getDecl();
-  if (Tag->getDeclName() == Name &&
-  Tag->getDeclContext()->getRedeclContext()
-  ->Equals(TD->getDeclContext()->getRedeclContext())) {
+  bool AnonymousEnumEligible =
+  getLangOpts().MSVCCompat && TUK == TUK_Reference &&
+  Kind == TTK_Enum && Tag->getDeclName().isEmpty();
+  if ((Tag->getDeclName() == Name || AnonymousEnumEligible) &&
+  Tag->getDeclContext()->getRedeclContext()->Equals(
+  TD->getDeclContext()->getRedeclContext())) {
 PrevDecl = Tag;
 Previous.clear();
 Previous.addDecl(Tag);


Index: clang/test/Sema/enum-typedef-msvc.cpp
===
--- /dev/null
+++ clang/test/Sema/enum-typedef-msvc.cpp
@@ -0,0 +1,18 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -fms-compatibility %s
+
+typedef enum {
+  First,
+  Second
+} MyEnum;
+
+class AMyInterface {
+  virtual void MyFunc(enum MyEnum *param) = 0;
+};
+
+class MyImpl : public AMyInterface {
+  virtual void MyFunc(enum MyEnum *param) override {}
+};
+
+enum MyEnum myEnum;
+
+// expected-no-diagnostics
Index: clang/test/Sema/enum-typedef-msvc.c
===
--- /dev/null
+++ clang/test/Sema/enum-typedef-msvc.c
@@ -0,0 +1,22 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fsyntax-only -verify -fms-compatibility -DUSE_MSVC_COMPAT %s
+
+typedef enum {
+  A
+} Foo;
+
+void func() {
+#ifdef USE_MSVC_COMPAT
+  enum Foo foo; // expected-no-diagnostics
+#else
+  enum Foo foo; // expected-error {{variable has incomplete type 'enum Foo'}} // expected-note {{forward 

[PATCH] D91659: Allow anonymous enum typedefs to be referenced with the 'enum' specifier under MSVC compat mode

[Shivanshu Goyal via Phabricator via cfe-commits]

shivanshu3 created this revision.
shivanshu3 added reviewers: zahen, tiagoma, rnk, hans.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
shivanshu3 requested review of this revision.

Goal: Clang should be able to parse the following code with 
'-fms-compatibility' because MSVC allows using the 'enum' specifier for 
typedef'd anonymous enums:

  typedef enum
  {
  First,
  Second
  } MyEnum;
  
  void func()
  {
  enum MyEnum foo;
  }

Today the code above produces the following compile error:

  :9:7: error: typedef 'MyEnum' cannot be referenced with a enum 
specifier
  enum MyEnum foo;
   ^
  :5:3: note: declared here
  } MyEnum;
^

The reason why this change is desired in Clang is because the MSVC tools 
'mktyplib' and 'midl' produce C++ code which uses the 'enum' specifier in such 
a way. Here is a small repro:

MyEnum.idl:

  [
uuid(da2889bf-6173-4c1c-a23d-52ad85fc91ca)
  ]
  library MyEnumLib
  {
typedef enum { X } MyEnum;
  };

Application.idl:

  [
uuid(4d0cc96f-3258-46f7-98bf-6654c1d027c5)
  ]
  library ApplicationLib
  {
importlib("MyEnum.tlb");
  
[
  uuid(6ac4c3d7-15c7-40c7-8b64-60459e29680b)
]
interface ApplicationInterface : IDispatch
{
  [
id(66),
propget
  ]
  HRESULT Foo([out, retval] enum MyEnum* Foo);
  
  [
id(66),
propput
  ]
  HRESULT Foo([in] enum MyEnum Foo);
};
  };

Use the following commands to build:

  mktyplib /win32 /cpp_cmd cl.exe /cpp_opt /E /tlb MyEnum.tlb /h MyEnum.h 
MyEnum.idl
  midl -h Application.h -iid Application.c -tlb Application.tlb /cpp_cmd cl.exe 
Application.idl

This produces Application.h with the following snippet (note the usage of the 
'enum' specifier):

  MIDL_INTERFACE("6ac4c3d7-15c7-40c7-8b64-60459e29680b")
  ApplicationInterface : public IDispatch
  {
  public:
  virtual /* [propget][id] */ HRESULT STDMETHODCALLTYPE get_Foo( 
  /* [retval][out] */ enum /* external definition not present */ MyEnum 
*Foo) = 0;
  
  virtual /* [propput][id] */ HRESULT STDMETHODCALLTYPE put_Foo( 
  /* [in] */ enum /* external definition not present */ MyEnum Foo) = 0;
  
  };

Note that the 'mktyplib' tool is deprecated now but it's used for building some 
code here at Microsoft, and probably other legacy build scenarios in the 
industry as well.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D91659

Files:
  clang/lib/Sema/SemaDecl.cpp
  clang/test/Sema/enum-typedef-msvc.c
  clang/test/Sema/enum-typedef-msvc.cpp


Index: clang/test/Sema/enum-typedef-msvc.cpp
===
--- /dev/null
+++ clang/test/Sema/enum-typedef-msvc.cpp
@@ -0,0 +1,16 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -fms-compatibility %s
+
+typedef enum {
+  First,
+  Second
+} MyEnum;
+
+class AMyInterface {
+  virtual void MyFunc(enum MyEnum *param) = 0;
+};
+
+class MyImpl : public AMyInterface {
+  virtual void MyFunc(enum MyEnum *param) override {}
+};
+
+// expected-no-diagnostics
Index: clang/test/Sema/enum-typedef-msvc.c
===
--- /dev/null
+++ clang/test/Sema/enum-typedef-msvc.c
@@ -0,0 +1,15 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fsyntax-only -verify -fms-compatibility -DUSE_MSVC_COMPAT 
%s
+
+typedef enum {
+  A
+} Foo;
+
+void func() {
+#ifdef USE_MSVC_COMPAT
+  enum Foo foo; // expected-no-diagnostics
+#else
+  enum Foo foo; // expected-error {{variable has incomplete type 'enum Foo'}} 
// expected-note {{forward declaration of 'enum Foo'}}
+#endif
+  (void)foo;
+}
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -15539,6 +15539,34 @@
 // shouldn't be diagnosing.
 LookupName(Previous, S);
 
+// Under MSVC, the 'enum' specifier can be used for typedef'd enums.
+// Note that lookup only fails in C, not C++, so this if condition
+// is only used for C code.
+if (getLangOpts().MSVCCompat && (Kind == TTK_Enum) && Previous.empty() &&
+(TUK == TUK_Reference)) {
+  LookupResult TypedefEnumLookup(*this, Name, NameLoc, LookupOrdinaryName,
+ Redecl);
+  LookupName(TypedefEnumLookup, S);
+
+  if (!TypedefEnumLookup.empty()) {
+if (TypedefNameDecl *TD =
+dyn_cast(TypedefEnumLookup.getFoundDecl())) {
+  const Type *UnderlyingTypePtr =
+  TD->getUnderlyingType().getTypePtrOrNull();
+
+  if (UnderlyingTypePtr) {
+if (EnumDecl *UnderlyingEnumDecl =
+dyn_cast(UnderlyingTypePtr->getAsTagDecl())) {
+  // We only allow this for 

[PATCH] D91129: Print source location in the error message when parens are missing around sizeof typename and the expression is inside macro expansion

[Shivanshu Goyal via Phabricator via cfe-commits]

shivanshu3 updated this revision to Diff 304973.
shivanshu3 edited the summary of this revision.
shivanshu3 added a comment.

Use getFileLoc instead of getExpansionLoc to get the location of error.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91129

Files:
  clang/lib/Parse/ParseExpr.cpp
  clang/test/Parser/sizeof-missing-parens.c


Index: clang/test/Parser/sizeof-missing-parens.c
===
--- /dev/null
+++ clang/test/Parser/sizeof-missing-parens.c
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+void Foo(int);
+
+#define Bar(x) Foo(x)
+
+void Baz()
+{
+   Foo(sizeof int); // expected-error {{expected parentheses around type 
name in sizeof expression}}
+   Bar(sizeof int); // expected-error {{expected parentheses around type 
name in sizeof expression}}
+}
Index: clang/lib/Parse/ParseExpr.cpp
===
--- clang/lib/Parse/ParseExpr.cpp
+++ clang/lib/Parse/ParseExpr.cpp
@@ -2278,12 +2278,20 @@
 Declarator DeclaratorInfo(DS, DeclaratorContext::TypeNameContext);
 ParseDeclarator(DeclaratorInfo);
 
-SourceLocation LParenLoc = PP.getLocForEndOfToken(OpTok.getLocation());
-SourceLocation RParenLoc = PP.getLocForEndOfToken(PrevTokLocation);
-Diag(LParenLoc, diag::err_expected_parentheses_around_typename)
-  << OpTok.getName()
-  << FixItHint::CreateInsertion(LParenLoc, "(")
-  << FixItHint::CreateInsertion(RParenLoc, ")");
+SourceLocation OpTokLoc = OpTok.getLocation();
+if (OpTokLoc.isMacroID()) {
+  SourceLocation OpTokExpansionLoc =
+  PP.getSourceManager().getFileLoc(OpTokLoc);
+  Diag(OpTokExpansionLoc,
+   diag::err_expected_parentheses_around_typename)
+  << OpTok.getName();
+} else {
+  SourceLocation LParenLoc = PP.getLocForEndOfToken(OpTokLoc);
+  SourceLocation RParenLoc = PP.getLocForEndOfToken(PrevTokLocation);
+  Diag(LParenLoc, diag::err_expected_parentheses_around_typename)
+  << OpTok.getName() << FixItHint::CreateInsertion(LParenLoc, "(")
+  << FixItHint::CreateInsertion(RParenLoc, ")");
+}
 isCastExpr = true;
 return ExprEmpty();
   }


Index: clang/test/Parser/sizeof-missing-parens.c
===
--- /dev/null
+++ clang/test/Parser/sizeof-missing-parens.c
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+void Foo(int);
+
+#define Bar(x) Foo(x)
+
+void Baz()
+{
+	Foo(sizeof int); // expected-error {{expected parentheses around type name in sizeof expression}}
+	Bar(sizeof int); // expected-error {{expected parentheses around type name in sizeof expression}}
+}
Index: clang/lib/Parse/ParseExpr.cpp
===
--- clang/lib/Parse/ParseExpr.cpp
+++ clang/lib/Parse/ParseExpr.cpp
@@ -2278,12 +2278,20 @@
 Declarator DeclaratorInfo(DS, DeclaratorContext::TypeNameContext);
 ParseDeclarator(DeclaratorInfo);
 
-SourceLocation LParenLoc = PP.getLocForEndOfToken(OpTok.getLocation());
-SourceLocation RParenLoc = PP.getLocForEndOfToken(PrevTokLocation);
-Diag(LParenLoc, diag::err_expected_parentheses_around_typename)
-  << OpTok.getName()
-  << FixItHint::CreateInsertion(LParenLoc, "(")
-  << FixItHint::CreateInsertion(RParenLoc, ")");
+SourceLocation OpTokLoc = OpTok.getLocation();
+if (OpTokLoc.isMacroID()) {
+  SourceLocation OpTokExpansionLoc =
+  PP.getSourceManager().getFileLoc(OpTokLoc);
+  Diag(OpTokExpansionLoc,
+   diag::err_expected_parentheses_around_typename)
+  << OpTok.getName();
+} else {
+  SourceLocation LParenLoc = PP.getLocForEndOfToken(OpTokLoc);
+  SourceLocation RParenLoc = PP.getLocForEndOfToken(PrevTokLocation);
+  Diag(LParenLoc, diag::err_expected_parentheses_around_typename)
+  << OpTok.getName() << FixItHint::CreateInsertion(LParenLoc, "(")
+  << FixItHint::CreateInsertion(RParenLoc, ")");
+}
 isCastExpr = true;
 return ExprEmpty();
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91129: Print source location in the error message when parens are missing around sizeof typename and the expression is inside macro expansion

[Shivanshu Goyal via Phabricator via cfe-commits]

shivanshu3 added a reviewer: tiagoma.
shivanshu3 added a comment.

Note that this regression seems to be there ever since Clang 3.4.1


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91129

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

[PATCH] D91129: Print source location in the error message when parens are missing around sizeof typename and the expression is inside macro expansion

[Shivanshu Goyal via Phabricator via cfe-commits]

shivanshu3 created this revision.
shivanshu3 added reviewers: zahen, hans, rsmith.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
shivanshu3 requested review of this revision.

Given the following code:

  void Foo(int);
  
  #define Bar(x) Foo(x)
  
  void Baz()
  {
Bar(sizeof int);
  }

The error message which is printed today is this:

  error: expected parentheses around type name in sizeof expression

There is no source location printed whatsoever, so fixing a compile break like 
this becomes extremely hard in a large codebase.

My change improves the error message. But it doesn't output a FixItHint because 
I wasn't able to figure out how to get the locations for left and right parens. 
So any tips would be appreciated.

  :7:2: error: expected parentheses around type name in sizeof 
expression
  Bar(sizeof int);
  ^


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D91129

Files:
  clang/lib/Parse/ParseExpr.cpp
  clang/test/Parser/sizeof-missing-parens.c


Index: clang/test/Parser/sizeof-missing-parens.c
===
--- /dev/null
+++ clang/test/Parser/sizeof-missing-parens.c
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+void Foo(int);
+
+#define Bar(x) Foo(x)
+
+void Baz()
+{
+   Foo(sizeof int); // expected-error {{expected parentheses around type 
name in sizeof expression}}
+   Bar(sizeof int); // expected-error {{expected parentheses around type 
name in sizeof expression}}
+}
Index: clang/lib/Parse/ParseExpr.cpp
===
--- clang/lib/Parse/ParseExpr.cpp
+++ clang/lib/Parse/ParseExpr.cpp
@@ -2278,12 +2278,20 @@
 Declarator DeclaratorInfo(DS, DeclaratorContext::TypeNameContext);
 ParseDeclarator(DeclaratorInfo);
 
-SourceLocation LParenLoc = PP.getLocForEndOfToken(OpTok.getLocation());
-SourceLocation RParenLoc = PP.getLocForEndOfToken(PrevTokLocation);
-Diag(LParenLoc, diag::err_expected_parentheses_around_typename)
-  << OpTok.getName()
-  << FixItHint::CreateInsertion(LParenLoc, "(")
-  << FixItHint::CreateInsertion(RParenLoc, ")");
+SourceLocation OpTokLoc = OpTok.getLocation();
+if (OpTokLoc.isMacroID()) {
+  SourceLocation OpTokExpansionLoc =
+  PP.getSourceManager().getExpansionLoc(OpTokLoc);
+  Diag(OpTokExpansionLoc,
+   diag::err_expected_parentheses_around_typename)
+  << OpTok.getName();
+} else {
+  SourceLocation LParenLoc = PP.getLocForEndOfToken(OpTokLoc);
+  SourceLocation RParenLoc = PP.getLocForEndOfToken(PrevTokLocation);
+  Diag(LParenLoc, diag::err_expected_parentheses_around_typename)
+  << OpTok.getName() << FixItHint::CreateInsertion(LParenLoc, "(")
+  << FixItHint::CreateInsertion(RParenLoc, ")");
+}
 isCastExpr = true;
 return ExprEmpty();
   }


Index: clang/test/Parser/sizeof-missing-parens.c
===
--- /dev/null
+++ clang/test/Parser/sizeof-missing-parens.c
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+void Foo(int);
+
+#define Bar(x) Foo(x)
+
+void Baz()
+{
+	Foo(sizeof int); // expected-error {{expected parentheses around type name in sizeof expression}}
+	Bar(sizeof int); // expected-error {{expected parentheses around type name in sizeof expression}}
+}
Index: clang/lib/Parse/ParseExpr.cpp
===
--- clang/lib/Parse/ParseExpr.cpp
+++ clang/lib/Parse/ParseExpr.cpp
@@ -2278,12 +2278,20 @@
 Declarator DeclaratorInfo(DS, DeclaratorContext::TypeNameContext);
 ParseDeclarator(DeclaratorInfo);
 
-SourceLocation LParenLoc = PP.getLocForEndOfToken(OpTok.getLocation());
-SourceLocation RParenLoc = PP.getLocForEndOfToken(PrevTokLocation);
-Diag(LParenLoc, diag::err_expected_parentheses_around_typename)
-  << OpTok.getName()
-  << FixItHint::CreateInsertion(LParenLoc, "(")
-  << FixItHint::CreateInsertion(RParenLoc, ")");
+SourceLocation OpTokLoc = OpTok.getLocation();
+if (OpTokLoc.isMacroID()) {
+  SourceLocation OpTokExpansionLoc =
+  PP.getSourceManager().getExpansionLoc(OpTokLoc);
+  Diag(OpTokExpansionLoc,
+   diag::err_expected_parentheses_around_typename)
+  << OpTok.getName();
+} else {
+  SourceLocation LParenLoc = PP.getLocForEndOfToken(OpTokLoc);
+  SourceLocation RParenLoc = PP.getLocForEndOfToken(PrevTokLocation);
+  Diag(LParenLoc, diag::err_expected_parentheses_around_typename)
+  << OpTok.getName() << FixItHint::CreateInsertion(LParenLoc, "(")
+  << FixItHint::CreateInsertion(RParenLoc, ")");
+}
 

[PATCH] D88680: Add ability to turn off -fpch-instantiate-templates in clang-cl

[Shivanshu Goyal via Phabricator via cfe-commits]

shivanshu3 updated this revision to Diff 296393.
shivanshu3 added a comment.

Address Hans' comments

- Prepend %s with "--" for clang-cl
- Remove a redundant test case


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88680

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/pch-instantiate-templates.c


Index: clang/test/Driver/pch-instantiate-templates.c
===
--- /dev/null
+++ clang/test/Driver/pch-instantiate-templates.c
@@ -0,0 +1,13 @@
+// CL driver test cases
+// RUN: %clang_cl -### /Yc /Fpfoo.pch /Fofoo.obj -- %s 2>&1 | FileCheck 
--check-prefix=CLANG_CL_YC %s
+// RUN: %clang_cl -### /Yc /Fpfoo.pch /Fofoo.obj 
-fno-pch-instantiate-templates -- %s 2>&1 | FileCheck 
--check-prefix=CLANG_CL_YC_DISABLE %s
+
+// CLANG_CL_YC: "-fpch-instantiate-templates"
+// CLANG_CL_YC_DISABLE-NOT: "-fpch-instantiate-templates"
+
+// GCC driver test cases
+// RUN: %clang -### -x c-header %s -o %t/foo.pch 2>&1 | FileCheck 
-check-prefix=GCC_DEFAULT %s
+// RUN: %clang -### -x c-header %s -o %t/foo.pch -fpch-instantiate-templates 
2>&1 | FileCheck -check-prefix=GCC_DEFAULT_ENABLE %s
+
+// GCC_DEFAULT-NOT: "-fpch-instantiate-templates"
+// GCC_DEFAULT_ENABLE: "-fpch-instantiate-templates"
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -1212,7 +1212,11 @@
 if (YcArg && JA.getKind() >= Action::PrecompileJobClass &&
 JA.getKind() <= Action::AssembleJobClass) {
   CmdArgs.push_back(Args.MakeArgString("-building-pch-with-obj"));
-  CmdArgs.push_back(Args.MakeArgString("-fpch-instantiate-templates"));
+  // -fpch-instantiate-templates is the default when creating
+  // precomp using /Yc
+  if (Args.hasFlag(options::OPT_fpch_instantiate_templates,
+   options::OPT_fno_pch_instantiate_templates, true))
+CmdArgs.push_back(Args.MakeArgString("-fpch-instantiate-templates"));
 }
 if (YcArg || YuArg) {
   StringRef ThroughHeader = YcArg ? YcArg->getValue() : YuArg->getValue();
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -1475,11 +1475,11 @@
   Group, Flags<[DriverOption]>;
 def fpch_instantiate_templates:
   Flag <["-"], "fpch-instantiate-templates">,
-  Group, Flags<[CC1Option]>,
+  Group, Flags<[CC1Option, CoreOption]>,
   HelpText<"Instantiate templates already while building a PCH">;
 def fno_pch_instantiate_templates:
   Flag <["-"], "fno-pch-instantiate-templates">,
-  Group, Flags<[CC1Option]>;
+  Group, Flags<[CC1Option, CoreOption]>;
 defm pch_codegen: OptInFFlag<"pch-codegen", "Generate ", "Do not generate ",
   "code for uses of this PCH that assumes an explicit object file will be 
built for the PCH">;
 defm pch_debuginfo: OptInFFlag<"pch-debuginfo", "Generate ", "Do not generate 
",


Index: clang/test/Driver/pch-instantiate-templates.c
===
--- /dev/null
+++ clang/test/Driver/pch-instantiate-templates.c
@@ -0,0 +1,13 @@
+// CL driver test cases
+// RUN: %clang_cl -### /Yc /Fpfoo.pch /Fofoo.obj -- %s 2>&1 | FileCheck --check-prefix=CLANG_CL_YC %s
+// RUN: %clang_cl -### /Yc /Fpfoo.pch /Fofoo.obj -fno-pch-instantiate-templates -- %s 2>&1 | FileCheck --check-prefix=CLANG_CL_YC_DISABLE %s
+
+// CLANG_CL_YC: "-fpch-instantiate-templates"
+// CLANG_CL_YC_DISABLE-NOT: "-fpch-instantiate-templates"
+
+// GCC driver test cases
+// RUN: %clang -### -x c-header %s -o %t/foo.pch 2>&1 | FileCheck -check-prefix=GCC_DEFAULT %s
+// RUN: %clang -### -x c-header %s -o %t/foo.pch -fpch-instantiate-templates 2>&1 | FileCheck -check-prefix=GCC_DEFAULT_ENABLE %s
+
+// GCC_DEFAULT-NOT: "-fpch-instantiate-templates"
+// GCC_DEFAULT_ENABLE: "-fpch-instantiate-templates"
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -1212,7 +1212,11 @@
 if (YcArg && JA.getKind() >= Action::PrecompileJobClass &&
 JA.getKind() <= Action::AssembleJobClass) {
   CmdArgs.push_back(Args.MakeArgString("-building-pch-with-obj"));
-  CmdArgs.push_back(Args.MakeArgString("-fpch-instantiate-templates"));
+  // -fpch-instantiate-templates is the default when creating
+  // precomp using /Yc
+  if (Args.hasFlag(options::OPT_fpch_instantiate_templates,
+   options::OPT_fno_pch_instantiate_templates, true))
+CmdArgs.push_back(Args.MakeArgString("-fpch-instantiate-templates"));
 }
 if (YcArg || YuArg) {
   StringRef 

[PATCH] D88680: Add ability to turn off -fpch-instantiate-templates in clang-cl

[Shivanshu Goyal via Phabricator via cfe-commits]

shivanshu3 updated this revision to Diff 296340.
shivanshu3 added a comment.

Address Reid's comments:

- Use hasFlag instead of hasArg
- Add test cases


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88680

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/pch-instantiate-templates.c


Index: clang/test/Driver/pch-instantiate-templates.c
===
--- /dev/null
+++ clang/test/Driver/pch-instantiate-templates.c
@@ -0,0 +1,14 @@
+// CL driver test cases
+// RUN: %clang_cl -### /Yc /Fpfoo.pch /Fofoo.obj %s 2>&1 | FileCheck 
--check-prefix=CLANG_CL_YC %s
+// RUN: %clang -### --driver-mode=cl /Yc /Fpfoo.pch /Fofoo.obj %s 2>&1 | 
FileCheck --check-prefix=CLANG_CL_YC %s
+// RUN: %clang_cl -### /Yc /Fpfoo.pch /Fofoo.obj 
-fno-pch-instantiate-templates %s 2>&1 | FileCheck 
--check-prefix=CLANG_CL_YC_DISABLE %s
+
+// CLANG_CL_YC: "-fpch-instantiate-templates"
+// CLANG_CL_YC_DISABLE-NOT: "-fpch-instantiate-templates"
+
+// GCC driver test cases
+// RUN: %clang -### -x c-header %s -o %t/foo.pch 2>&1 | FileCheck 
-check-prefix=GCC_DEFAULT %s
+// RUN: %clang -### -x c-header %s -o %t/foo.pch -fpch-instantiate-templates 
2>&1 | FileCheck -check-prefix=GCC_DEFAULT_ENABLE %s
+
+// GCC_DEFAULT-NOT: "-fpch-instantiate-templates"
+// GCC_DEFAULT_ENABLE: "-fpch-instantiate-templates"
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -1212,7 +1212,11 @@
 if (YcArg && JA.getKind() >= Action::PrecompileJobClass &&
 JA.getKind() <= Action::AssembleJobClass) {
   CmdArgs.push_back(Args.MakeArgString("-building-pch-with-obj"));
-  CmdArgs.push_back(Args.MakeArgString("-fpch-instantiate-templates"));
+  // -fpch-instantiate-templates is the default when creating
+  // precomp using /Yc
+  if (Args.hasFlag(options::OPT_fpch_instantiate_templates,
+   options::OPT_fno_pch_instantiate_templates, true))
+CmdArgs.push_back(Args.MakeArgString("-fpch-instantiate-templates"));
 }
 if (YcArg || YuArg) {
   StringRef ThroughHeader = YcArg ? YcArg->getValue() : YuArg->getValue();
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -1475,11 +1475,11 @@
   Group, Flags<[DriverOption]>;
 def fpch_instantiate_templates:
   Flag <["-"], "fpch-instantiate-templates">,
-  Group, Flags<[CC1Option]>,
+  Group, Flags<[CC1Option, CoreOption]>,
   HelpText<"Instantiate templates already while building a PCH">;
 def fno_pch_instantiate_templates:
   Flag <["-"], "fno-pch-instantiate-templates">,
-  Group, Flags<[CC1Option]>;
+  Group, Flags<[CC1Option, CoreOption]>;
 defm pch_codegen: OptInFFlag<"pch-codegen", "Generate ", "Do not generate ",
   "code for uses of this PCH that assumes an explicit object file will be 
built for the PCH">;
 defm pch_debuginfo: OptInFFlag<"pch-debuginfo", "Generate ", "Do not generate 
",


Index: clang/test/Driver/pch-instantiate-templates.c
===
--- /dev/null
+++ clang/test/Driver/pch-instantiate-templates.c
@@ -0,0 +1,14 @@
+// CL driver test cases
+// RUN: %clang_cl -### /Yc /Fpfoo.pch /Fofoo.obj %s 2>&1 | FileCheck --check-prefix=CLANG_CL_YC %s
+// RUN: %clang -### --driver-mode=cl /Yc /Fpfoo.pch /Fofoo.obj %s 2>&1 | FileCheck --check-prefix=CLANG_CL_YC %s
+// RUN: %clang_cl -### /Yc /Fpfoo.pch /Fofoo.obj -fno-pch-instantiate-templates %s 2>&1 | FileCheck --check-prefix=CLANG_CL_YC_DISABLE %s
+
+// CLANG_CL_YC: "-fpch-instantiate-templates"
+// CLANG_CL_YC_DISABLE-NOT: "-fpch-instantiate-templates"
+
+// GCC driver test cases
+// RUN: %clang -### -x c-header %s -o %t/foo.pch 2>&1 | FileCheck -check-prefix=GCC_DEFAULT %s
+// RUN: %clang -### -x c-header %s -o %t/foo.pch -fpch-instantiate-templates 2>&1 | FileCheck -check-prefix=GCC_DEFAULT_ENABLE %s
+
+// GCC_DEFAULT-NOT: "-fpch-instantiate-templates"
+// GCC_DEFAULT_ENABLE: "-fpch-instantiate-templates"
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -1212,7 +1212,11 @@
 if (YcArg && JA.getKind() >= Action::PrecompileJobClass &&
 JA.getKind() <= Action::AssembleJobClass) {
   CmdArgs.push_back(Args.MakeArgString("-building-pch-with-obj"));
-  CmdArgs.push_back(Args.MakeArgString("-fpch-instantiate-templates"));
+  // -fpch-instantiate-templates is the default when creating
+  // precomp using /Yc
+  if (Args.hasFlag(options::OPT_fpch_instantiate_templates,
+   

[PATCH] D88680: Add ability to turn off -fpch-instantiate-templates in clang-cl

[Shivanshu Goyal via Phabricator via cfe-commits]

shivanshu3 added a comment.

Also, I wanted to mention that clang-cl.exe actually crashes deterministically 
when running precompiles for 2 of our headers when using 
'-fpch-instantiate-templates' with the following signature:

  1.   parser at end of file
  2.  Per-file LLVM IR generation
  3.  {msvc}\lib\native\include\xmemory:807:41: Generating code for 
declaration 'std::allocator::allocate'
  4.  {msvc}\lib\native\include\xmemory:44:30: LLVM IR generation of 
declaration 'std::_New_alignof'

Once I'm able to get a smaller repro which I can share, I will update you guys 
with it. @llunak FYI


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88680

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

[PATCH] D88680: Add ability to turn off -fpch-instantiate-templates in clang-cl

[Shivanshu Goyal via Phabricator via cfe-commits]

shivanshu3 added a comment.

> Can you get at least some testcase, even if not small?

Yes, in a few days I can get you the test case. I'll have to figure out what 
our company policy is for sharing code and if I should obfuscate it so I don't 
get into trouble :)

> MSVC does some kind of template instantiation for PCHs too

MSVC's cl.exe compiles the precomp header with an empty .cpp file, which is 
where the template instantiations occur, and which is also the reason why the 
precomp header has to be self contained. So yes, in principle your change 
should be safe when enabled by default for clang-cl but there is a bug 
somewhere which we should figure out.

@llunak


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88680

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

[PATCH] D88680: Add ability to turn off -fpch-instantiate-templates in clang-cl

[Shivanshu Goyal via Phabricator via cfe-commits]

shivanshu3 added a comment.

In D88680#2307564 , @rnk wrote:

> I think the flag was originally intended to be an internal -cc1 flag not 
> exposed to users. You should be able to work around your problem with 
> `-Xclang -fno-pch-instantiate-templates`, btw.
>
> 
>
> @zequanwu, can you patch this in locally and take over this patch? Please 
> address the hasFlag comment above, add a test to clang/test/Driver, and make 
> sure the flag works with `--driver-mode=gcc` and `--driver-mode=cl`. Follow 
> the examples of the other tests, run clang with `-###`, and make sure this 
> flag does or does not appear on the `-cc1` line.

Should we keep this flag internal though, or is it OK to make it a core flag? 
Our codebase had hundreds of compile errors caused by this default behavior 
which was hard to track down, so I'm guessing other people compiling with 
clang-cl might face similar problems? Or maybe we should re-think about making 
this the default behavior for clang-cl. Personally I don't have any strong 
preferences because we have a workaround now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88680

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

[PATCH] D88680: Add ability to turn off -fpch-instantiate-templates in clang-cl

[Shivanshu Goyal via Phabricator via cfe-commits]

shivanshu3 added a comment.

Note that I do not have commit access and this change will have to be committed 
by someone else on my behalf. Please use the following commit author details:
"Shivan Goyal "


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88680

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

[PATCH] D88680: Add ability to turn off -fpch-instantiate-templates in clang-cl

[Shivanshu Goyal via Phabricator via cfe-commits]

shivanshu3 created this revision.
shivanshu3 added reviewers: llunak, rnk, rsmith, hans, zahen.
shivanshu3 added a project: clang.
Herald added subscribers: cfe-commits, dang.
shivanshu3 requested review of this revision.

A lot of our code building with clang-cl.exe using Clang 11 was failing with 
the following 2 type of errors:

1. explicit specialization of 'foo' after instantiation
2. no matching function for call to 'bar'

Note that we also use `-fdelayed-template-parsing` in our builds.

I tried pretty hard to get a small repro for these failures, but couldn't. So 
there is some subtle edge case in the `-fpch-instantiate-templates` feature 
introduced by this change:
https://reviews.llvm.org/D69585

When I tried turning this off using `-fno-pch-instantiate-templates`, builds 
would silently fail with the same error without any indication that 
`-fno-pch-instantiate-templates` was being ignored by the compiler. Then I 
realized this "no" option wasn't actually working when I ran Clang under a 
debugger.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D88680

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Clang.cpp


Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -1212,7 +1212,10 @@
 if (YcArg && JA.getKind() >= Action::PrecompileJobClass &&
 JA.getKind() <= Action::AssembleJobClass) {
   CmdArgs.push_back(Args.MakeArgString("-building-pch-with-obj"));
-  CmdArgs.push_back(Args.MakeArgString("-fpch-instantiate-templates"));
+  // -fpch-instantiate-templates is the default when creating
+  // precomp using /Yc
+  if (!Args.hasArg(options::OPT_fno_pch_instantiate_templates))
+CmdArgs.push_back(Args.MakeArgString("-fpch-instantiate-templates"));
 }
 if (YcArg || YuArg) {
   StringRef ThroughHeader = YcArg ? YcArg->getValue() : YuArg->getValue();
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -1475,11 +1475,11 @@
   Group, Flags<[DriverOption]>;
 def fpch_instantiate_templates:
   Flag <["-"], "fpch-instantiate-templates">,
-  Group, Flags<[CC1Option]>,
+  Group, Flags<[CC1Option, CoreOption]>,
   HelpText<"Instantiate templates already while building a PCH">;
 def fno_pch_instantiate_templates:
   Flag <["-"], "fno-pch-instantiate-templates">,
-  Group, Flags<[CC1Option]>;
+  Group, Flags<[CC1Option, CoreOption]>;
 defm pch_codegen: OptInFFlag<"pch-codegen", "Generate ", "Do not generate ",
   "code for uses of this PCH that assumes an explicit object file will be 
built for the PCH">;
 defm pch_debuginfo: OptInFFlag<"pch-debuginfo", "Generate ", "Do not generate 
",


Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -1212,7 +1212,10 @@
 if (YcArg && JA.getKind() >= Action::PrecompileJobClass &&
 JA.getKind() <= Action::AssembleJobClass) {
   CmdArgs.push_back(Args.MakeArgString("-building-pch-with-obj"));
-  CmdArgs.push_back(Args.MakeArgString("-fpch-instantiate-templates"));
+  // -fpch-instantiate-templates is the default when creating
+  // precomp using /Yc
+  if (!Args.hasArg(options::OPT_fno_pch_instantiate_templates))
+CmdArgs.push_back(Args.MakeArgString("-fpch-instantiate-templates"));
 }
 if (YcArg || YuArg) {
   StringRef ThroughHeader = YcArg ? YcArg->getValue() : YuArg->getValue();
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -1475,11 +1475,11 @@
   Group, Flags<[DriverOption]>;
 def fpch_instantiate_templates:
   Flag <["-"], "fpch-instantiate-templates">,
-  Group, Flags<[CC1Option]>,
+  Group, Flags<[CC1Option, CoreOption]>,
   HelpText<"Instantiate templates already while building a PCH">;
 def fno_pch_instantiate_templates:
   Flag <["-"], "fno-pch-instantiate-templates">,
-  Group, Flags<[CC1Option]>;
+  Group, Flags<[CC1Option, CoreOption]>;
 defm pch_codegen: OptInFFlag<"pch-codegen", "Generate ", "Do not generate ",
   "code for uses of this PCH that assumes an explicit object file will be built for the PCH">;
 defm pch_debuginfo: OptInFFlag<"pch-debuginfo", "Generate ", "Do not generate ",
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D86999: getClangStripDependencyFileAdjuster(): Do not remove -M args when using MSVC cl driver

[Shivanshu Goyal via Phabricator via cfe-commits]

shivanshu3 marked an inline comment as done.
shivanshu3 added a comment.

Note that I do not have commit access and this change will have to be committed 
by someone else on my behalf. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86999

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

[PATCH] D86999: getClangStripDependencyFileAdjuster(): Do not remove -M args when using MSVC cl driver

[Shivanshu Goyal via Phabricator via cfe-commits]

shivanshu3 marked 2 inline comments as done.
shivanshu3 added inline comments.



Comment at: clang/lib/Tooling/ArgumentsAdjusters.cpp:117
+  // do not remove those when using the cl driver.
+  bool IsDependencyFileArg;
+  if (Arg.startswith("/showIncludes") || Arg.startswith("-showIncludes"))

hans wrote:
> shivanshu3 wrote:
> > hans wrote:
> > > Instead of a bool and if below, I'd suggest if-statements and early 
> > > continues instead. Breaking up the old if-statement is nice though, and 
> > > maybe the comment from above about what flags to ignore could be moved 
> > > down here to those if statements. For example:
> > > 
> > > 
> > > ```
> > > // -M flags blah blah
> > > if (Arg.startswith("-M") && !UsingClDriver)
> > >   continue;
> > > // MSVC flags blah blah
> > > if (Arg.startswith("/showIncludes") || Arg.startswith("-showIncludes"))
> > >   continue;
> > > AdjustedArgs.push_back(Args[i]);
> > > ```
> > I realized that with my original change, we would skip the next argument 
> > under cl driver mode when -MT was used, which would be wrong. The next 
> > argument should only be skipped when `IsDependencyFileArg` is true. So I 
> > think it might be cleaner to keep that extra boolean so the code is easy to 
> > read and understand. What do you think?
> I think having the boolean variable is still more confusing (it adds more 
> state to keep track of) than just handling the cases with if-statements and 
> early continue.
> 
> How about:
> 
> ```
> // -M flags that take an arg..
> if (!UsingClDriver && (Arg == "-MF" || Arg == "-MT" || Arg == "-MQ")) {
>   ++i;
>   continue;
> }
> // -M flags blah blah
> if (!UsingClDriver && Arg.startswith("-M"))
>   continue;
> // MSVC flags blah blah
> if (Arg.startswith("/showIncludes") || Arg.startswith("-showIncludes"))
>   continue;
> 
> AdjustedArgs.push_back(Args[i]);
> ```
> 
> ?
Yup I agree that looks better :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86999

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

[PATCH] D86999: getClangStripDependencyFileAdjuster(): Do not remove -M args when using MSVC cl driver

[Shivanshu Goyal via Phabricator via cfe-commits]

shivanshu3 updated this revision to Diff 289793.
shivanshu3 added a comment.

- Remove the bool `IsDependencyFileArg` in the implementation of 
`getClangStripDependencyFileAdjuster()` to make it simpler.
- Add an extra argument after -MT in the test case to ensure we do not strip 
arguments after -MT when using the cl driver mode.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86999

Files:
  clang/lib/Tooling/ArgumentsAdjusters.cpp
  clang/unittests/Tooling/ToolingTest.cpp

Index: clang/unittests/Tooling/ToolingTest.cpp
===
--- clang/unittests/Tooling/ToolingTest.cpp
+++ clang/unittests/Tooling/ToolingTest.cpp
@@ -563,6 +563,40 @@
   EXPECT_TRUE(HasFlag("-c"));
 }
 
+// Check getClangStripDependencyFileAdjuster doesn't strip args when using the
+// MSVC cl.exe driver
+TEST(ClangToolTest, StripDependencyFileAdjusterMsvc) {
+  FixedCompilationDatabase Compilations(
+  "/", {"--driver-mode=cl", "-MD", "-MDd", "-MT", "-O1", "-MTd", "-MP"});
+
+  ClangTool Tool(Compilations, std::vector(1, "/a.cc"));
+  Tool.mapVirtualFile("/a.cc", "void a() {}");
+
+  std::unique_ptr Action(
+  newFrontendActionFactory());
+
+  CommandLineArguments FinalArgs;
+  ArgumentsAdjuster CheckFlagsAdjuster =
+  [](const CommandLineArguments , StringRef /*unused*/) {
+FinalArgs = Args;
+return Args;
+  };
+  Tool.clearArgumentsAdjusters();
+  Tool.appendArgumentsAdjuster(getClangStripDependencyFileAdjuster());
+  Tool.appendArgumentsAdjuster(CheckFlagsAdjuster);
+  Tool.run(Action.get());
+
+  auto HasFlag = [](const std::string ) {
+return llvm::find(FinalArgs, Flag) != FinalArgs.end();
+  };
+  EXPECT_TRUE(HasFlag("-MD"));
+  EXPECT_TRUE(HasFlag("-MDd"));
+  EXPECT_TRUE(HasFlag("-MT"));
+  EXPECT_TRUE(HasFlag("-O1"));
+  EXPECT_TRUE(HasFlag("-MTd"));
+  EXPECT_TRUE(HasFlag("-MP"));
+}
+
 // Check getClangStripPluginsAdjuster strips plugin related args.
 TEST(ClangToolTest, StripPluginsAdjuster) {
   FixedCompilationDatabase Compilations(
Index: clang/lib/Tooling/ArgumentsAdjusters.cpp
===
--- clang/lib/Tooling/ArgumentsAdjusters.cpp
+++ clang/lib/Tooling/ArgumentsAdjusters.cpp
@@ -21,6 +21,16 @@
 namespace clang {
 namespace tooling {
 
+static StringRef getDriverMode(const CommandLineArguments ) {
+  for (const auto  : Args) {
+StringRef ArgRef = Arg;
+if (ArgRef.consume_front("--driver-mode=")) {
+  return ArgRef;
+}
+  }
+  return StringRef();
+}
+
 /// Add -fsyntax-only option and drop options that triggers output generation.
 ArgumentsAdjuster getClangSyntaxOnlyAdjuster() {
   return [](const CommandLineArguments , StringRef /*unused*/) {
@@ -93,20 +103,28 @@
 
 ArgumentsAdjuster getClangStripDependencyFileAdjuster() {
   return [](const CommandLineArguments , StringRef /*unused*/) {
+auto UsingClDriver = (getDriverMode(Args) == "cl");
+
 CommandLineArguments AdjustedArgs;
 for (size_t i = 0, e = Args.size(); i < e; ++i) {
   StringRef Arg = Args[i];
-  // All dependency-file options begin with -M. These include -MM,
-  // -MF, -MG, -MP, -MT, -MQ, -MD, and -MMD.
-  if (!Arg.startswith("-M") && !Arg.startswith("/showIncludes") &&
-  !Arg.startswith("-showIncludes")) {
-AdjustedArgs.push_back(Args[i]);
+
+  // These flags take an argument: -MX foo. Skip the next argument also.
+  if (!UsingClDriver && (Arg == "-MF" || Arg == "-MT" || Arg == "-MQ")) {
+++i;
 continue;
   }
+  // When not using the cl driver mode, dependency file generation options
+  // begin with -M. These include -MM, -MF, -MG, -MP, -MT, -MQ, -MD, and
+  // -MMD.
+  if (!UsingClDriver && Arg.startswith("-M"))
+continue;
+  // Under MSVC's cl driver mode, dependency file generation is controlled
+  // using /showIncludes
+  if (Arg.startswith("/showIncludes") || Arg.startswith("-showIncludes"))
+continue;
 
-  if (Arg == "-MF" || Arg == "-MT" || Arg == "-MQ")
-// These flags take an argument: -MX foo. Skip the next argument also.
-++i;
+  AdjustedArgs.push_back(Args[i]);
 }
 return AdjustedArgs;
   };
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D86999: getClangStripDependencyFileAdjuster(): Do not remove -M args when using MSVC cl driver

[Shivanshu Goyal via Phabricator via cfe-commits]

shivanshu3 marked 5 inline comments as done.
shivanshu3 added inline comments.



Comment at: clang/lib/Tooling/ArgumentsAdjusters.cpp:117
+  // do not remove those when using the cl driver.
+  bool IsDependencyFileArg;
+  if (Arg.startswith("/showIncludes") || Arg.startswith("-showIncludes"))

hans wrote:
> Instead of a bool and if below, I'd suggest if-statements and early continues 
> instead. Breaking up the old if-statement is nice though, and maybe the 
> comment from above about what flags to ignore could be moved down here to 
> those if statements. For example:
> 
> 
> ```
> // -M flags blah blah
> if (Arg.startswith("-M") && !UsingClDriver)
>   continue;
> // MSVC flags blah blah
> if (Arg.startswith("/showIncludes") || Arg.startswith("-showIncludes"))
>   continue;
> AdjustedArgs.push_back(Args[i]);
> ```
I realized that with my original change, we would skip the next argument under 
cl driver mode when -MT was used, which would be wrong. The next argument 
should only be skipped when `IsDependencyFileArg` is true. So I think it might 
be cleaner to keep that extra boolean so the code is easy to read and 
understand. What do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86999

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

[PATCH] D86999: getClangStripDependencyFileAdjuster(): Do not remove -M args when using MSVC cl driver

[Shivanshu Goyal via Phabricator via cfe-commits]

shivanshu3 updated this revision to Diff 289606.
shivanshu3 added a comment.

- Simplified the implementation of `getDriverMode` and got rid of the 
`Optional` return type.
- When using the cl driver mode, we do not want to skip the next argument for 
-MF, -MT, -MQ.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86999

Files:
  clang/lib/Tooling/ArgumentsAdjusters.cpp
  clang/unittests/Tooling/ToolingTest.cpp

Index: clang/unittests/Tooling/ToolingTest.cpp
===
--- clang/unittests/Tooling/ToolingTest.cpp
+++ clang/unittests/Tooling/ToolingTest.cpp
@@ -563,6 +563,39 @@
   EXPECT_TRUE(HasFlag("-c"));
 }
 
+// Check getClangStripDependencyFileAdjuster doesn't strip args when using the
+// MSVC cl.exe driver
+TEST(ClangToolTest, StripDependencyFileAdjusterMsvc) {
+  FixedCompilationDatabase Compilations(
+  "/", {"--driver-mode=cl", "-MD", "-MDd", "-MT", "-MTd", "-MP"});
+
+  ClangTool Tool(Compilations, std::vector(1, "/a.cc"));
+  Tool.mapVirtualFile("/a.cc", "void a() {}");
+
+  std::unique_ptr Action(
+  newFrontendActionFactory());
+
+  CommandLineArguments FinalArgs;
+  ArgumentsAdjuster CheckFlagsAdjuster =
+  [](const CommandLineArguments , StringRef /*unused*/) {
+FinalArgs = Args;
+return Args;
+  };
+  Tool.clearArgumentsAdjusters();
+  Tool.appendArgumentsAdjuster(getClangStripDependencyFileAdjuster());
+  Tool.appendArgumentsAdjuster(CheckFlagsAdjuster);
+  Tool.run(Action.get());
+
+  auto HasFlag = [](const std::string ) {
+return llvm::find(FinalArgs, Flag) != FinalArgs.end();
+  };
+  EXPECT_TRUE(HasFlag("-MD"));
+  EXPECT_TRUE(HasFlag("-MDd"));
+  EXPECT_TRUE(HasFlag("-MT"));
+  EXPECT_TRUE(HasFlag("-MTd"));
+  EXPECT_TRUE(HasFlag("-MP"));
+}
+
 // Check getClangStripPluginsAdjuster strips plugin related args.
 TEST(ClangToolTest, StripPluginsAdjuster) {
   FixedCompilationDatabase Compilations(
Index: clang/lib/Tooling/ArgumentsAdjusters.cpp
===
--- clang/lib/Tooling/ArgumentsAdjusters.cpp
+++ clang/lib/Tooling/ArgumentsAdjusters.cpp
@@ -21,6 +21,16 @@
 namespace clang {
 namespace tooling {
 
+static StringRef getDriverMode(const CommandLineArguments ) {
+  for (const auto  : Args) {
+StringRef ArgRef = Arg;
+if (ArgRef.consume_front("--driver-mode=")) {
+  return ArgRef;
+}
+  }
+  return StringRef();
+}
+
 /// Add -fsyntax-only option and drop options that triggers output generation.
 ArgumentsAdjuster getClangSyntaxOnlyAdjuster() {
   return [](const CommandLineArguments , StringRef /*unused*/) {
@@ -93,20 +103,33 @@
 
 ArgumentsAdjuster getClangStripDependencyFileAdjuster() {
   return [](const CommandLineArguments , StringRef /*unused*/) {
+auto UsingClDriver = (getDriverMode(Args) == "cl");
+
 CommandLineArguments AdjustedArgs;
 for (size_t i = 0, e = Args.size(); i < e; ++i) {
   StringRef Arg = Args[i];
-  // All dependency-file options begin with -M. These include -MM,
-  // -MF, -MG, -MP, -MT, -MQ, -MD, and -MMD.
-  if (!Arg.startswith("-M") && !Arg.startswith("/showIncludes") &&
-  !Arg.startswith("-showIncludes")) {
-AdjustedArgs.push_back(Args[i]);
-continue;
-  }
 
-  if (Arg == "-MF" || Arg == "-MT" || Arg == "-MQ")
+  bool IsDependencyFileArg;
+  // Under MSVC's cl driver mode, dependency file generation is controlled
+  // using /showIncludes
+  if (Arg.startswith("/showIncludes") || Arg.startswith("-showIncludes"))
+IsDependencyFileArg = true;
+  else if (UsingClDriver)
+IsDependencyFileArg = false;
+  // When not using the cl driver mode, dependency file generation options
+  // begin with -M. These include -MM, -MF, -MG, -MP, -MT, -MQ, -MD, and
+  // -MMD.
+  else if (Arg.startswith("-M"))
+IsDependencyFileArg = true;
+  else
+IsDependencyFileArg = false;
+
+  if (!IsDependencyFileArg) {
+AdjustedArgs.push_back(Args[i]);
+  } else if ((Arg == "-MF" || Arg == "-MT" || Arg == "-MQ")) {
 // These flags take an argument: -MX foo. Skip the next argument also.
 ++i;
+  }
 }
 return AdjustedArgs;
   };
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D86999: getClangStripDependencyFileAdjuster(): Do not remove -M args when using MSVC cl driver

[Shivanshu Goyal via Phabricator via cfe-commits]

shivanshu3 created this revision.
shivanshu3 added reviewers: aeubanks, kadircet.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
shivanshu3 requested review of this revision.
Herald added a subscriber: ormris.

MSVC's cl.exe has a few command line arguments which start with -M such as 
"-MD", "-MDd", "-MT", "-MTd", "-MP".
These arguments are not dependency file generation related, and these arguments 
were being removed by getClangStripDependencyFileAdjuster() which was wrong.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D86999

Files:
  clang/lib/Tooling/ArgumentsAdjusters.cpp
  clang/unittests/Tooling/ToolingTest.cpp


Index: clang/unittests/Tooling/ToolingTest.cpp
===
--- clang/unittests/Tooling/ToolingTest.cpp
+++ clang/unittests/Tooling/ToolingTest.cpp
@@ -563,6 +563,39 @@
   EXPECT_TRUE(HasFlag("-c"));
 }
 
+// Check getClangStripDependencyFileAdjuster doesn't strip args when using the
+// MSVC cl.exe driver
+TEST(ClangToolTest, StripDependencyFileAdjusterMsvc) {
+  FixedCompilationDatabase Compilations(
+  "/", {"--driver-mode=cl", "-MD", "-MDd", "-MT", "-MTd", "-MP"});
+
+  ClangTool Tool(Compilations, std::vector(1, "/a.cc"));
+  Tool.mapVirtualFile("/a.cc", "void a() {}");
+
+  std::unique_ptr Action(
+  newFrontendActionFactory());
+
+  CommandLineArguments FinalArgs;
+  ArgumentsAdjuster CheckFlagsAdjuster =
+  [](const CommandLineArguments , StringRef /*unused*/) {
+FinalArgs = Args;
+return Args;
+  };
+  Tool.clearArgumentsAdjusters();
+  Tool.appendArgumentsAdjuster(getClangStripDependencyFileAdjuster());
+  Tool.appendArgumentsAdjuster(CheckFlagsAdjuster);
+  Tool.run(Action.get());
+
+  auto HasFlag = [](const std::string ) {
+return llvm::find(FinalArgs, Flag) != FinalArgs.end();
+  };
+  EXPECT_TRUE(HasFlag("-MD"));
+  EXPECT_TRUE(HasFlag("-MDd"));
+  EXPECT_TRUE(HasFlag("-MT"));
+  EXPECT_TRUE(HasFlag("-MTd"));
+  EXPECT_TRUE(HasFlag("-MP"));
+}
+
 // Check getClangStripPluginsAdjuster strips plugin related args.
 TEST(ClangToolTest, StripPluginsAdjuster) {
   FixedCompilationDatabase Compilations(
Index: clang/lib/Tooling/ArgumentsAdjusters.cpp
===
--- clang/lib/Tooling/ArgumentsAdjusters.cpp
+++ clang/lib/Tooling/ArgumentsAdjusters.cpp
@@ -21,6 +21,17 @@
 namespace clang {
 namespace tooling {
 
+static Optional getDriverMode(const CommandLineArguments ) {
+  for (const auto  : Args) {
+StringRef ArgRef = Arg;
+if (ArgRef.startswith("--driver-mode=")) {
+  auto EqualSignIndex = ArgRef.find('=');
+  return StringRef(ArgRef.data() + EqualSignIndex + 1);
+}
+  }
+  return StringRef();
+}
+
 /// Add -fsyntax-only option and drop options that triggers output generation.
 ArgumentsAdjuster getClangSyntaxOnlyAdjuster() {
   return [](const CommandLineArguments , StringRef /*unused*/) {
@@ -93,13 +104,27 @@
 
 ArgumentsAdjuster getClangStripDependencyFileAdjuster() {
   return [](const CommandLineArguments , StringRef /*unused*/) {
+auto DriverMode = getDriverMode(Args);
+auto UsingClDriver = (DriverMode.hasValue() && *DriverMode == "cl");
+
 CommandLineArguments AdjustedArgs;
 for (size_t i = 0, e = Args.size(); i < e; ++i) {
   StringRef Arg = Args[i];
   // All dependency-file options begin with -M. These include -MM,
   // -MF, -MG, -MP, -MT, -MQ, -MD, and -MMD.
-  if (!Arg.startswith("-M") && !Arg.startswith("/showIncludes") &&
-  !Arg.startswith("-showIncludes")) {
+  // Dependency-file options in MSVC do not begin with -M, so we
+  // do not remove those when using the cl driver.
+  bool IsDependencyFileArg;
+  if (Arg.startswith("/showIncludes") || Arg.startswith("-showIncludes"))
+IsDependencyFileArg = true;
+  else if (UsingClDriver)
+IsDependencyFileArg = false;
+  else if (Arg.startswith("-M"))
+IsDependencyFileArg = true;
+  else
+IsDependencyFileArg = false;
+
+  if (!IsDependencyFileArg) {
 AdjustedArgs.push_back(Args[i]);
 continue;
   }


Index: clang/unittests/Tooling/ToolingTest.cpp
===
--- clang/unittests/Tooling/ToolingTest.cpp
+++ clang/unittests/Tooling/ToolingTest.cpp
@@ -563,6 +563,39 @@
   EXPECT_TRUE(HasFlag("-c"));
 }
 
+// Check getClangStripDependencyFileAdjuster doesn't strip args when using the
+// MSVC cl.exe driver
+TEST(ClangToolTest, StripDependencyFileAdjusterMsvc) {
+  FixedCompilationDatabase Compilations(
+  "/", {"--driver-mode=cl", "-MD", "-MDd", "-MT", "-MTd", "-MP"});
+
+  ClangTool Tool(Compilations, std::vector(1, "/a.cc"));
+  Tool.mapVirtualFile("/a.cc", "void a() {}");
+
+  std::unique_ptr Action(
+  newFrontendActionFactory());
+
+  CommandLineArguments FinalArgs;
+  ArgumentsAdjuster CheckFlagsAdjuster =
+  [](const