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

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

It needs tests for the warnings about badly formed pragmas, and for pragmas outside file/namespace scope.



================
Comment at: clang/lib/Parse/ParsePragma.cpp:3561
         << "intrinsic";
     return;
   }
----------------
steplong wrote:
> hans wrote:
> > since the above is just a warning, we should probably still call the ActOn.. method?
> Hmm, I'm not sure because all the msgs above are also warnings (diag::warn_)
Yes, but for those, (missing left paren, or unknown intrinsic) it makes sense to do nothing. If it's just the right paren missing, I don't think it makes sense to ignore the intrinsic? Can you check how we handle similar situations with other intrinsics?


================
Comment at: clang/lib/Sema/SemaAttr.cpp:1079
+  MSFunctionNoBuiltins.insert(MSFunctionNoBuiltins.end(),
+                              NoBuiltins.begin(), NoBuiltins.end());
+}
----------------
steplong wrote:
> hans wrote:
> > Do we want to avoid duplicates in MSFunctionNoBuiltins? Or maybe it doesn't matter?
> Yea, I didn't think it really mattered. I originally wanted to use a set, but I needed the strings to be stored in contiguous memory for NoBuitinAttr::CreateImplicit() in Sema::AddRangeBasedNoBuiltin()
A set would be nicer conceptually of course.

How about using an llvm::SmallSetVector. That will solve the problem of duplicates, it will be deterministic, and you can use getArrayRef() to get the values in contiguous memory, or maybe being() and end() will work too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124702