[PATCH] D44630: [ms] Parse #pragma optimize and ignore it behind its own flag

[Hans Wennborg via Phabricator via cfe-commits]

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



Comment at: include/clang/Basic/DiagnosticParseKinds.td:973
+def warn_pragma_optimize : Warning<
+  "'#pragma optimize' is not supported; use '#pragma clang optimize on|off' 
instead">,
+  InGroup;

thakis wrote:
> Is `pragma clang optimize` really what we want to recommend here? `pragma 
> optimize` is used in practice mostly to work around cl.exe compiler bugs, or 
> to disable inlining. In neither case, `pragma clang optimize` is what you'd 
> really want to use. Maybe just omit everything after ; and instead add a 
> blurb about this in DiagnosticDocs.td ?
I'll drop the suggestion for now.

Turns out it was not so easy to get a blurb into DiagnosticDocs or 
DiagnosticsReference. They're auto-generated but don't seem to have a good way 
to enter extra documentation. I'll just leave it for now.


https://reviews.llvm.org/D44630



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

[PATCH] D44630: [ms] Parse #pragma optimize and ignore it behind its own flag

[Hans Wennborg via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
hans marked an inline comment as done.
Closed by commit rL327959: [ms] Parse #pragma optimize and ignore it behind its 
own flag (authored by hans, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D44630?vs=138940=139087#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D44630

Files:
  cfe/trunk/include/clang/Basic/DiagnosticGroups.td
  cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td
  cfe/trunk/include/clang/Parse/Parser.h
  cfe/trunk/lib/Parse/ParsePragma.cpp
  cfe/trunk/test/Preprocessor/pragma_microsoft.c

Index: cfe/trunk/test/Preprocessor/pragma_microsoft.c
===
--- cfe/trunk/test/Preprocessor/pragma_microsoft.c
+++ cfe/trunk/test/Preprocessor/pragma_microsoft.c
@@ -190,3 +190,11 @@
 #pragma intrinsic(asdf) // no-warning
 #pragma clang diagnostic pop
 #pragma intrinsic(asdf) // expected-warning {{'asdf' is not a recognized builtin; consider including }}
+
+#pragma optimize  // expected-warning{{missing '(' after '#pragma optimize'}}
+#pragma optimize( // expected-warning{{expected string literal in '#pragma optimize'}}
+#pragma optimize(a// expected-warning{{expected string literal in '#pragma optimize'}}
+#pragma optimize("g"  // expected-warning{{expected ',' in '#pragma optimize'}}
+#pragma optimize("g", // expected-warning{{missing argument to '#pragma optimize'; expected 'on' or 'off'}}
+#pragma optimize("g",xyz  // expected-warning{{unexpected argument 'xyz' to '#pragma optimize'; expected 'on' or 'off'}}
+#pragma optimize("g",on)  // expected-warning{{#pragma optimize' is not supported}}
Index: cfe/trunk/lib/Parse/ParsePragma.cpp
===
--- cfe/trunk/lib/Parse/ParsePragma.cpp
+++ cfe/trunk/lib/Parse/ParsePragma.cpp
@@ -220,6 +220,12 @@
 Token ) override;
 };
 
+struct PragmaMSOptimizeHandler : public PragmaHandler {
+  PragmaMSOptimizeHandler() : PragmaHandler("optimize") {}
+  void HandlePragma(Preprocessor , PragmaIntroducerKind Introducer,
+Token ) override;
+};
+
 struct PragmaForceCUDAHostDeviceHandler : public PragmaHandler {
   PragmaForceCUDAHostDeviceHandler(Sema )
   : PragmaHandler("force_cuda_host_device"), Actions(Actions) {}
@@ -324,6 +330,8 @@
 PP.AddPragmaHandler(MSRuntimeChecks.get());
 MSIntrinsic.reset(new PragmaMSIntrinsicHandler());
 PP.AddPragmaHandler(MSIntrinsic.get());
+MSOptimize.reset(new PragmaMSOptimizeHandler());
+PP.AddPragmaHandler(MSOptimize.get());
   }
 
   if (getLangOpts().CUDA) {
@@ -410,6 +418,8 @@
 MSRuntimeChecks.reset();
 PP.RemovePragmaHandler(MSIntrinsic.get());
 MSIntrinsic.reset();
+PP.RemovePragmaHandler(MSOptimize.get());
+MSOptimize.reset();
   }
 
   if (getLangOpts().CUDA) {
@@ -2949,6 +2959,61 @@
 PP.Diag(Tok.getLocation(), diag::warn_pragma_extra_tokens_at_eol)
 << "intrinsic";
 }
+
+// #pragma optimize("gsty", on|off)
+void PragmaMSOptimizeHandler::HandlePragma(Preprocessor ,
+   PragmaIntroducerKind Introducer,
+   Token ) {
+  SourceLocation StartLoc = Tok.getLocation();
+  PP.Lex(Tok);
+
+  if (Tok.isNot(tok::l_paren)) {
+PP.Diag(Tok.getLocation(), diag::warn_pragma_expected_lparen) << "optimize";
+return;
+  }
+  PP.Lex(Tok);
+
+  if (Tok.isNot(tok::string_literal)) {
+PP.Diag(Tok.getLocation(), diag::warn_pragma_expected_string) << "optimize";
+return;
+  }
+  // We could syntax check the string but it's probably not worth the effort.
+  PP.Lex(Tok);
+
+  if (Tok.isNot(tok::comma)) {
+PP.Diag(Tok.getLocation(), diag::warn_pragma_expected_comma) << "optimize";
+return;
+  }
+  PP.Lex(Tok);
+
+  if (Tok.is(tok::eod) || Tok.is(tok::r_paren)) {
+PP.Diag(Tok.getLocation(), diag::warn_pragma_missing_argument)
+<< "optimize" << /*Expected=*/true << "'on' or 'off'";
+return;
+  }
+  IdentifierInfo *II = Tok.getIdentifierInfo();
+  if (!II || (!II->isStr("on") && !II->isStr("off"))) {
+PP.Diag(Tok.getLocation(), diag::warn_pragma_invalid_argument)
+<< PP.getSpelling(Tok) << "optimize" << /*Expected=*/true
+<< "'on' or 'off'";
+return;
+  }
+  PP.Lex(Tok);
+
+  if (Tok.isNot(tok::r_paren)) {
+PP.Diag(Tok.getLocation(), diag::warn_pragma_expected_rparen) << "optimize";
+return;
+  }
+  PP.Lex(Tok);
+
+  if (Tok.isNot(tok::eod)) {
+PP.Diag(Tok.getLocation(), diag::warn_pragma_extra_tokens_at_eol)
+<< "optimize";
+return;
+  }
+  PP.Diag(StartLoc, diag::warn_pragma_optimize);
+}
+
 void PragmaForceCUDAHostDeviceHandler::HandlePragma(
 Preprocessor , PragmaIntroducerKind Introducer, Token ) {
   Token FirstTok = Tok;
Index: cfe/trunk/include/clang/Parse/Parser.h

[PATCH] D44630: [ms] Parse #pragma optimize and ignore it behind its own flag

[Nico Weber via Phabricator via cfe-commits]

thakis accepted this revision.
thakis added a comment.

Awesome, thanks! One nit below:




Comment at: include/clang/Basic/DiagnosticParseKinds.td:973
+def warn_pragma_optimize : Warning<
+  "'#pragma optimize' is not supported; use '#pragma clang optimize on|off' 
instead">,
+  InGroup;

Is `pragma clang optimize` really what we want to recommend here? `pragma 
optimize` is used in practice mostly to work around cl.exe compiler bugs, or to 
disable inlining. In neither case, `pragma clang optimize` is what you'd really 
want to use. Maybe just omit everything after ; and instead add a blurb about 
this in DiagnosticDocs.td ?


https://reviews.llvm.org/D44630



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

[PATCH] D44630: [ms] Parse #pragma optimize and ignore it behind its own flag

[Reid Kleckner via Phabricator via cfe-commits]

rnk accepted this revision.
rnk added a comment.
This revision is now accepted and ready to land.

lgtm, thanks!




Comment at: lib/Parse/ParsePragma.cpp:2970
+
+  if (Tok.isNot(tok::l_paren)) {
+PP.Diag(Tok.getLocation(), diag::warn_pragma_expected_lparen) << 
"optimize";

One day, someone ought to refactor pragma parsing to be less verbose and error 
prone, but today is not that day.


https://reviews.llvm.org/D44630



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

[PATCH] D44630: [ms] Parse #pragma optimize and ignore it behind its own flag

[Hans Wennborg via Phabricator via cfe-commits]

hans created this revision.
hans added reviewers: thakis, rnk.

This allows users to turn off warnings about this pragma specifically, while 
still receiving warnings about other ignored pragmas.


https://reviews.llvm.org/D44630

Files:
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Basic/DiagnosticParseKinds.td
  include/clang/Parse/Parser.h
  lib/Parse/ParsePragma.cpp
  test/Preprocessor/pragma_microsoft.c

Index: test/Preprocessor/pragma_microsoft.c
===
--- test/Preprocessor/pragma_microsoft.c
+++ test/Preprocessor/pragma_microsoft.c
@@ -190,3 +190,11 @@
 #pragma intrinsic(asdf) // no-warning
 #pragma clang diagnostic pop
 #pragma intrinsic(asdf) // expected-warning {{'asdf' is not a recognized builtin; consider including }}
+
+#pragma optimize  // expected-warning{{missing '(' after '#pragma optimize'}}
+#pragma optimize( // expected-warning{{expected string literal in '#pragma optimize'}}
+#pragma optimize(a// expected-warning{{expected string literal in '#pragma optimize'}}
+#pragma optimize("g"  // expected-warning{{expected ',' in '#pragma optimize'}}
+#pragma optimize("g", // expected-warning{{missing argument to '#pragma optimize'; expected 'on' or 'off'}}
+#pragma optimize("g",xyz  // expected-warning{{unexpected argument 'xyz' to '#pragma optimize'; expected 'on' or 'off'}}
+#pragma optimize("g",on)  // expected-warning{{#pragma optimize' is not supported; use '#pragma clang optimize on|off' instead}}
Index: lib/Parse/ParsePragma.cpp
===
--- lib/Parse/ParsePragma.cpp
+++ lib/Parse/ParsePragma.cpp
@@ -220,6 +220,12 @@
 Token ) override;
 };
 
+struct PragmaMSOptimizeHandler : public PragmaHandler {
+  PragmaMSOptimizeHandler() : PragmaHandler("optimize") {}
+  void HandlePragma(Preprocessor , PragmaIntroducerKind Introducer,
+Token ) override;
+};
+
 struct PragmaForceCUDAHostDeviceHandler : public PragmaHandler {
   PragmaForceCUDAHostDeviceHandler(Sema )
   : PragmaHandler("force_cuda_host_device"), Actions(Actions) {}
@@ -324,6 +330,8 @@
 PP.AddPragmaHandler(MSRuntimeChecks.get());
 MSIntrinsic.reset(new PragmaMSIntrinsicHandler());
 PP.AddPragmaHandler(MSIntrinsic.get());
+MSOptimize.reset(new PragmaMSOptimizeHandler());
+PP.AddPragmaHandler(MSOptimize.get());
   }
 
   if (getLangOpts().CUDA) {
@@ -410,6 +418,8 @@
 MSRuntimeChecks.reset();
 PP.RemovePragmaHandler(MSIntrinsic.get());
 MSIntrinsic.reset();
+PP.RemovePragmaHandler(MSOptimize.get());
+MSOptimize.reset();
   }
 
   if (getLangOpts().CUDA) {
@@ -2949,6 +2959,61 @@
 PP.Diag(Tok.getLocation(), diag::warn_pragma_extra_tokens_at_eol)
 << "intrinsic";
 }
+
+// #pragma optimize("gsty", on|off)
+void PragmaMSOptimizeHandler::HandlePragma(Preprocessor ,
+   PragmaIntroducerKind Introducer,
+   Token ) {
+  SourceLocation StartLoc = Tok.getLocation();
+  PP.Lex(Tok);
+
+  if (Tok.isNot(tok::l_paren)) {
+PP.Diag(Tok.getLocation(), diag::warn_pragma_expected_lparen) << "optimize";
+return;
+  }
+  PP.Lex(Tok);
+
+  if (Tok.isNot(tok::string_literal)) {
+PP.Diag(Tok.getLocation(), diag::warn_pragma_expected_string) << "optimize";
+return;
+  }
+  // We could syntax check the string but it's probably not worth the effort.
+  PP.Lex(Tok);
+
+  if (Tok.isNot(tok::comma)) {
+PP.Diag(Tok.getLocation(), diag::warn_pragma_expected_comma) << "optimize";
+return;
+  }
+  PP.Lex(Tok);
+
+  if (Tok.is(tok::eod) || Tok.is(tok::r_paren)) {
+PP.Diag(Tok.getLocation(), diag::warn_pragma_missing_argument)
+<< "optimize" << /*Expected=*/true << "'on' or 'off'";
+return;
+  }
+  IdentifierInfo *II = Tok.getIdentifierInfo();
+  if (!II || (!II->isStr("on") && !II->isStr("off"))) {
+PP.Diag(Tok.getLocation(), diag::warn_pragma_invalid_argument)
+<< PP.getSpelling(Tok) << "optimize" << /*Expected=*/true
+<< "'on' or 'off'";
+return;
+  }
+  PP.Lex(Tok);
+
+  if (Tok.isNot(tok::r_paren)) {
+PP.Diag(Tok.getLocation(), diag::warn_pragma_expected_rparen) << "optimize";
+return;
+  }
+  PP.Lex(Tok);
+
+  if (Tok.isNot(tok::eod)) {
+PP.Diag(Tok.getLocation(), diag::warn_pragma_extra_tokens_at_eol)
+<< "optimize";
+return;
+  }
+  PP.Diag(StartLoc, diag::warn_pragma_optimize);
+}
+
 void PragmaForceCUDAHostDeviceHandler::HandlePragma(
 Preprocessor , PragmaIntroducerKind Introducer, Token ) {
   Token FirstTok = Tok;
Index: include/clang/Parse/Parser.h
===
--- include/clang/Parse/Parser.h
+++ include/clang/Parse/Parser.h
@@ -179,6 +179,7 @@
   std::unique_ptr MSSection;
   std::unique_ptr MSRuntimeChecks;
   std::unique_ptr MSIntrinsic;
+  std::unique_ptr MSOptimize;