[PATCH] D68028: [clang] Add no_builtin attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/Attr.td:3416
+
+def NoBuiltin : InheritableAttr {
+  let Spellings = [Clang<"no_builtin">];
----------------
I think we do not want this to be an inheritable attribute, but just `Attr` instead, because this can only be written on definitions (so there's no way to inherit the attribute from previous declarations).


================
Comment at: clang/test/Sema/no-builtin.c:26
+int __attribute__((no_builtin("*"))) variable;
+// expected-warning at -1 {{'no_builtin' attribute only applies to functions}}
----------------
gchatelet wrote:
> aaron.ballman wrote:
> > You should probably add another test case for this situation:
> > ```
> > void whatever() __attribute__((no_builtin("*", "*"))) {}
> > ```
> > and for member functions in C++ as well:
> > ```
> > struct S {
> >   void whatever() __attribute__((no_builtin("memcpy"))) {}
> >   virtual void intentional() __attribute__((no_builtin("memcpy"))) {}
> > };
> > ```
> ```
> void whatever() __attribute__((no_builtin("*", "*"))) {}
> ```
> 
> I've added a few ones.
> 
> ```
> struct S {
>   void whatever() __attribute__((no_builtin("memcpy"))) {}
>   virtual void intentional() __attribute__((no_builtin("memcpy"))) {}
> };
> ```
> 
> What do you want me to test for this one?
> What do you want me to test for this one?

Ensuring that applying the attribute to member function definitions, including virtual ones, is supported and doesn't cause diagnostics. The codegen side of things will test that overridden virtual methods do not inherit the attribute.

Actually, there's another interesting test hiding in there for when we do want a diagnostic (I think):
```
struct S {
  void whatever() __attribute__((no_builtin("memcpy"))); // Should diagnose as not a definition
};
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68028