[PATCH] D124702: [MSVC] Add support for pragma function

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:988
+def err_pragma_intrinsic_function_scope : Error<
+  "'#pragma function/intrinsic' can only appear outside a function, at the global level">;
+
----------------



================
Comment at: clang/include/clang/Sema/Sema.h:758
 
+  /// Set of no-builtin functions listed by \#pragma function
+  llvm::SmallSetVector<StringRef, 4> MSFunctionNoBuiltins;
----------------



================
Comment at: clang/include/clang/Sema/Sema.h:10329-10334
+  void ActOnPragmaMSIntrinsic(
+      SourceLocation Loc, const llvm::SmallSetVector<StringRef, 4> &Intrinsics);
+
+  /// Call on well formed \#pragma function.
+  void ActOnPragmaMSFunction(
+      SourceLocation Loc, const llvm::SmallSetVector<StringRef, 4> &NoBuiltins);
----------------
This makes it easier for callers who have a SmallVector of a different extent but the correct type.


================
Comment at: clang/lib/Parse/ParsePragma.cpp:3569
+
+  Actions.ActOnPragmaMSIntrinsic(FirstTok.getLocation(), Intrinsics);
+}
----------------
Given that this patch is about adding pragma function, I think it'd be better to split the changes to the `intrinsic` pragma out into their own patch set to keep a clear separation.


================
Comment at: clang/lib/Parse/ParsePragma.cpp:3572-3574
+void PragmaMSFunctionHandler::HandlePragma(Preprocessor &PP,
+                                            PragmaIntroducer Introducer,
+                                            Token &Tok) {
----------------
Formatting looks off here -- you should run your patch through clang-format (https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting).


================
Comment at: clang/lib/Parse/ParsePragma.cpp:3578
+
+  if (Tok.isNot(tok::l_paren)) {
+    PP.Diag(Tok.getLocation(), diag::warn_pragma_expected_lparen)
----------------
Can we use `ExpectAndConsume()` here instead?


================
Comment at: clang/lib/Parse/ParsePragma.cpp:3602-3607
+  if (Tok.isNot(tok::r_paren)) {
+    PP.Diag(Tok.getLocation(), diag::warn_pragma_expected_rparen)
+        << "function";
+    return;
+  }
+  PP.Lex(Tok); // )
----------------
Another `ExpectAndConsume()`?


================
Comment at: clang/lib/Sema/SemaAttr.cpp:1070
+    SourceLocation Loc, const llvm::SmallSetVector<StringRef, 4> &Intrinsics) {
+  if (!CurContext->getRedeclContext()->isFileContext()) {
+    Diag(Loc, diag::err_pragma_intrinsic_function_scope);
----------------
What is `CurContext` when this gets called for your .c test file? I would have expected it to be the `TranslationUnitDecl` which should be a file context (getting the redecl context shouldn't do anything in the cases I've seen).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124702