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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
