[PATCH] D68028: [clang] Add no_builtin attribute

[Guillaume Chatelet via Phabricator via cfe-commits]

gchatelet added a comment.

@aaron.ballman thx a lot for the review. I really appreciate it, especially because I'm not too familiar with this part of the codebase.



================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1092
+  // Wildcard is a super set of all builtins, we keep only this one.
+  if (FunctionNames.count(Wildcard) > 0) {
+    FunctionNames.clear();
----------------
aaron.ballman wrote:
> This silently changes the behavior from what the user might expect, which seems bad. For instance, it would be very reasonable for a user to write `[[clang::no_builtin("__builtin_mem*")]]` expecting that to behave as a regex pattern, but instead it silently turns into a "disable all builtins".
> 
> I think it makes sense to diagnose unexpected input like that rather than silently accept it under perhaps unexpected semantics. It may also make sense to support regex patterns in the strings or at least keep the design such that we can add that functionality later without breaking users.
The code looks for a function name that is exactly `*` and not "a function name that contains `*`", it would not turn `[[clang::no_builtin("__builtin_mem*")]]` into `[[clang::no_builtin("*")]]`.
That said, I fully agree that an error should be returned if the function name is not valid.
I'll work on this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68028