gchatelet marked 7 inline comments as done.
gchatelet added a comment.
thx @aaron.ballman , I'm waiting for your reply before uploading the new patch
(some of my comments won't have the accompanying code update sorry)
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1072
+NoBuiltinAttr *
+Sema::mergeNoBuiltinAttr(Sema &S, Decl *D, const AttributeCommonInfo &CI,
+ llvm::ArrayRef<StringRef> FunctionNames) {
----------------
aaron.ballman wrote:
> You're missing a call to this function within `mergeDeclAttribute()` in
> SemaDecl.cpp.
Thx, I rearranged the signature a bit, do you happen to know how
`mergeDeclAttribute` is tested?
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1078-1079
+ // Insert previous NoBuiltin attributes.
+ if (D->hasAttr<NoBuiltinAttr>())
+ for (StringRef FunctionName : D->getAttr<NoBuiltinAttr>()->functionNames())
+ FunctionNamesSet.insert(FunctionName);
----------------
aaron.ballman wrote:
> Instead of doing `hasAttr<>` followed by `getAttr<>`, this should be:
> ```
> if (const auto *NBA = D->getAttr<NoBuiltinAttr>()) {
> for (StringRef FunctionName : NBA->functionNames())
> ...
> }
> ```
> But are you able to use `llvm::copy()` instead of a manual loop?
I had to use a vector instead of a set and //uniquify// by hand.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1086-1089
+ if (FunctionNamesSet.count(Wildcard) > 0) {
+ FunctionNamesSet.clear();
+ FunctionNamesSet.insert(Wildcard);
+ }
----------------
aaron.ballman wrote:
> Rather than walking the entire set like this, would it make more sense to
> look for the wildcard in the above loop before inserting the name, and set a
> local variable if found, so that you can do the clear without having to
> rescan the entire list?
This is is conflict with the `llvm::copy` suggestion above. Which one do you
prefer?
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1098-1099
+
+ if (D->hasAttr<NoBuiltinAttr>())
+ D->dropAttr<NoBuiltinAttr>();
+
----------------
aaron.ballman wrote:
> Just making sure I understand the semantics you want: redeclarations do not
> have to have matching lists of builtins, but instead the definition will use
> a merged list? e.g.,
> ```
> [[clang::no_builtin("memset")]] void whatever();
> [[clang::no_builtin("memcpy")]] void whatever();
>
> [[clang::no_builtin("memmove")]] void whatever() {
> // Will not use memset, memcpy, or memmove builtins.
> }
> ```
> That seems a bit strange, to me. In fact, being able to apply this attribute
> to a declaration seems a bit like a mistake -- this only impacts the
> definition of the function, and I can't imagine good things coming from
> hypothetical code like:
> ```
> [[clang::no_builtin("memset")]] void whatever();
> #include "whatever.h" // Provides a library declaration of whatever() with no
> restrictions on it
> ```
> WDYT about restricting this attribute to only appear on definitions?
That's a very good point. Thx for noticing.
Indeed I think it only makes sense to have the attribute on the function
definition.
I've tried to to use `FunctionDecl->hasBody()` during attribute handling in the
Sema phase but it seems like the `FunctionDecl` is not complete at this point.
All calls to `hasBody()` return `false`, if I repeat the operation in `CGCall`
then `hasBody` returns `true` and I can see the `CompoundStatement`.
Do you have any recommendations on where to perform the check?
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
