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

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.



>> And should we error/warn if the pragma occurs not in namespace scope?
>
> Oh I wasn't sure how to check if the function pragma was outside of a function.

In the ActOn.. methods I think you can check what the CurContext is.



================
Comment at: clang/lib/Parse/ParsePragma.cpp:3542
 
+  std::vector<StringRef> Intrinsics;
   while (Tok.is(tok::identifier)) {
----------------
SmallVector would be more common LLVM style, I think. The ActOn.. methods could take the argument as a const-ref to SmallVectorImpl<StringRef>. This applies to the function pragma below as well.


================
Comment at: clang/lib/Parse/ParsePragma.cpp:3561
         << "intrinsic";
     return;
   }
----------------
since the above is just a warning, we should probably still call the ActOn.. method?


================
Comment at: clang/lib/Parse/ParsePragma.cpp:3605
+        << "function";
+    return;
+  }
----------------
same as above: since it's just a warning, maybe we still want to call the ActOn.. method?


================
Comment at: clang/lib/Sema/SemaAttr.cpp:1079
+  MSFunctionNoBuiltins.insert(MSFunctionNoBuiltins.end(),
+                              NoBuiltins.begin(), NoBuiltins.end());
+}
----------------
Do we want to avoid duplicates in MSFunctionNoBuiltins? Or maybe it doesn't matter?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124702