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