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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/Sema/SemaAttr.cpp:1070
+    SourceLocation Loc, const llvm::SmallVectorImpl<StringRef> &NoBuiltins) {
+  if (!CurContext->getRedeclContext()->isFileContext()) {
+    Diag(Loc, diag::err_pragma_intrinsic_function_scope) << "function";
----------------
Now that we solved the mystery of the context -- do we need `getRedeclContext()` here?


================
Comment at: clang/lib/Sema/SemaAttr.cpp:1099
 
+void Sema::AddRangeBasedNoBuiltin(FunctionDecl *FD) {
+  SmallVector<StringRef> V(MSFunctionNoBuiltins.begin(),
----------------
I think we should rename this function to be more specific to the pragma. How about `AddImplicitMSFunctionNoBuiltinAttr()` -- it's a mouthful, but it's at least a descriptive mouthful that won't be called too often.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:10224
+  if (D.isFunctionDefinition()) {
     AddRangeBasedOptnone(NewFD);
+    AddRangeBasedNoBuiltin(NewFD);
----------------
I'd be delighted if a follow-up NFC commit changed this name as well -- "RangeBased" doesn't tell me much of anything in these identifiers (it leaves me wondering why there's no range passed in to the function).


================
Comment at: clang/test/Preprocessor/pragma_microsoft.c:210-211
+#pragma function(main)                   // expected-warning {{'main' is not a recognized builtin; consider including <intrin.h>}}
+#pragma function(                        // expected-warning {{missing ')' after}}
+#pragma function(int)                    // expected-warning {{missing ')' after}}
+#pragma function(strcmp) asdf            // expected-warning {{extra tokens at end}}
----------------
It's a bit unfortunate that these don't suggest that an identifier is what's missing given that empty parens isn't a particularly useful pragma, however, I don't insist on a change either.


================
Comment at: clang/test/Preprocessor/pragma_microsoft.c:220
+
+void pragma_function_foo() {
+#pragma function(memset) // expected-error {{'#pragma function' can only appear at file scope}}
----------------
It'd be pretty reasonable to add the `struct` test here as well with a comment that Microsoft accepts it but we reject it on purpose with an appeal to the MS docs that say it must appear at file scope.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124702