aaron.ballman added a reviewer: aaron.ballman.
aaron.ballman added a comment.
Thank you for working on this!
It looks like you're missing all of the sema tests that check that the
attribute only appertains to functions, accepts the proper kind of arguments,
etc.
================
Comment at: clang/include/clang/Basic/Attr.td:3349
+ let Spellings = [Clang<"no_builtin">];
+ let Args = [VariadicStringArgument<"FunctionNames">];
+ let Subjects = SubjectList<[Function]>;
----------------
The documentation should explain what this is.
================
Comment at: clang/lib/CodeGen/CGCall.cpp:1853
+ if (const auto *Attr = TargetDecl->getAttr<NoBuiltinAttr>()) {
+ const auto HasWildcard = llvm::is_contained(Attr->functionNames(), "*");
+ assert(!HasWildcard ||
----------------
`const auto` -> `bool` (and drop the top-level `const`).
================
Comment at: clang/lib/CodeGen/CGCall.cpp:1859
+ else
+ for (const auto &FunctionName : Attr->functionNames()) {
+ SmallString<32> AttributeName;
----------------
`const auto &` -> `StringRef`?
================
Comment at: clang/lib/CodeGen/CGCall.cpp:1861-1862
+ SmallString<32> AttributeName;
+ // TODO: check that function names are valid for the
+ // TargetLibraryInfo.
+ AttributeName += "no-builtin-";
----------------
I think this checking should happen in Sema rather than CodeGen.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1075-1078
+ // Insert previous NoBuiltin attributes.
+ if (D->hasAttr<NoBuiltinAttr>())
+ for (StringRef FunctionName : D->getAttr<NoBuiltinAttr>()->functionNames())
+ FunctionNames.insert(FunctionName);
----------------
I think that this should be done in a `merge` function; there are plenty of
examples of how this is typically done elsewhere in the file.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1080-1081
+
+ if (AL.getNumArgs() == 0)
+ FunctionNames.insert(Wildcard);
+ else
----------------
Given that the user has to provide the parens for the attribute anyway, I think
this situation should be diagnosed rather than defaulting to the wildcard. It
helps catch think-os where the user put in parens and forgot to add the
parameter in the first place (the wildcard is not onerous to spell out).
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1091
+
+ // Wildcard is a super set of all builtins, we keep only this one.
+ if (FunctionNames.count(Wildcard) > 0) {
----------------
super set -> superset
================
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();
----------------
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.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1097
+
+ auto UniqueFunctionNames = FunctionNames.takeVector();
+ llvm::sort(UniqueFunctionNames);
----------------
Please don't use `auto` here as the type is not spelled out in the
initialization.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1100-1101
+
+ if (D->hasAttr<NoBuiltinAttr>())
+ D->dropAttr<NoBuiltinAttr>();
+
----------------
I think this needs to happen in a `merge` function.
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
