aaron.ballman added inline comments.
================
Comment at: clang/include/clang/Basic/Attr.td:1796
+// trigger an error.
+def RISCVOverlayCall : InheritableAttr {
+ let Spellings = [GCC<"overlaycall">];
----------------
jrtc27 wrote:
> aaron.ballman wrote:
> > edward-jones wrote:
> > > jrtc27 wrote:
> > > > If you want this to be portable to non-GNU compilers you should
> > > > consider using a keyword instead (which can still map to an attribute
> > > > internally). That also tends to get you better errors (there are places
> > > > where type attributes can get silently ignored currently).
> > > I don't think much consideration has been given to other compilers, but
> > > would it be unreasonable for the interface to this feature to not
> > > necessarily be identical between GNU and non-GNU compilers?
> > >
> > > That said, I'm happy to switch to a keyword, especially if as you mention
> > > there are cases where an attribute can silently go missing without error.
> > > I'm not sure on the distinction of when to use a keyword vs attribute
> > > though, given that keywords are used pretty sparingly in comparison.
> > This isn't a type attribute, so I don't think there's a risk of the
> > attribute getting lost.
> This one isn't, but the data one is
Oh, derp, thank you for catching that, somehow I completely missed it! I was
thrown off by there being a lot of problems with it, but all the problems made
it look like a declaration attribute.
Type attributes come with their own fun set of questions, like: does the
attribute impact the type for overloading? SFINAE? Name mangling?
================
Comment at: clang/include/clang/Basic/Attr.td:1805
+ let Spellings = [GCC<"overlaydata">];
+ let Subjects = SubjectList<[GlobalConst]>;
+ let Documentation = [RISCVOverlayDocs];
----------------
This subject list is incorrect -- the subject is a `VarDecl`, but that's not a
type.
================
Comment at: clang/include/clang/Basic/AttrDocs.td:2153
+
+``__attribute__((overlaydata))`` is used to mark global constants and signifies
+that the global is in an overlay and can only be accessed using the overlay
----------------
This description is wrong if this is intended to be a type attribute. A global
constant is not a type.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:8512-8517
+ if (!S.getLangOpts().RISCVOverlayFunctions) {
+ AL.setInvalid();
+ S.Diag(AL.getLoc(), diag::err_overlaydata_unsupported);
+ break;
+ }
+ D->addAttr(::new (S.Context) RISCVOverlayDataAttr(S.Context, AL));
----------------
aaron.ballman wrote:
> Please write a `handle` function for this as with all the other attributes
> (we have hopes to someday do more tablegen in this area, so sticking with the
> same pattern in this `switch` is strongly preferred).
Oops, this is entirely wrong -- if this is a type attribute, it should be
handled from `processTypeAttrs()` in `SemaType.cpp`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D109372/new/
https://reviews.llvm.org/D109372
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
