aaron.ballman added inline comments.

================
Comment at: clang/include/clang/Basic/Attr.td:2134
+def SwiftBridge : Attr {
+  let Spellings = [GNU<"swift_bridge">];
+  let Args = [StringArgument<"SwiftType">];
----------------
Is it intentional that this is `swift_bridge` but we just added 
`swift_bridged_typedef` (bridge vs bridged)? That seems like a bit of a 
spelling gotcha, if it can be corrected.


================
Comment at: clang/include/clang/Basic/Attr.td:2133
 
+def SwiftBridge : Attr {
+  let Spellings = [GNU<"swift_bridge">];
----------------
compnerd wrote:
> aaron.ballman wrote:
> > Is this a type or a declaration attribute? It looks like a declaration 
> > attribute based on the declaration and the list of subjects, but it looks 
> > like a type based on the `ExpectedType` diagnostic and the documentation. 
> > Or is this one of those unholy GNU attributes that's confused about what it 
> > appertains to?
> > 
> > Should this be inherited by redeclarations? Might be worth adding a test:
> > ```
> > struct __attribute__((swift_bridge)) S;
> > 
> > struct S { // Should still have the attribute
> >   int i;
> > };
> > ```
> It is a declaration attribute, and yes, it should be inheritable.
Okay, you should mark the attribute as an `InheritableAttr` above and a test.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:3483
+  let Content = [{
+The ``swift_bridged`` attribute indicates that the type to which the attribute
+appertains is bridged to the named Swift type.
----------------
aaron.ballman wrote:
> If this is a type attribute, should it be listed as a `TypeAttr` above?
The documentation should probably also say that it is bridging the declarations 
rather than the types, just to be clear. Maybe something like: The 
`swift_bridge` attribute indicates that the declaration to which the attribute 
is applied is bridged to the named Swift type. ?

If not that formulation, you should at least change `swift_bridged` to be 
`swift_bridge`.

Adding a code example of how to use this properly would also be appreciated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87532

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to