zixuw accepted this revision.
zixuw added inline comments.
This revision is now accepted and ready to land.
================
Comment at: clang/lib/ExtractAPI/ExtractAPIVisitor.cpp:174-175
+ StringRef Name = Decl->getName();
if (Name.empty())
Name = getTypedefName(Decl);
+ if (Name.empty()) {
----------------
dang wrote:
> zixuw wrote:
> > dang wrote:
> > > zixuw wrote:
> > > > Aren't these two lines supposed to do this?
> > > Qualified name is never empty, just contains some version of anonymous,
> > > whereas getName() is actually empty for anonymous types. The flow is now,
> > > try to populate with `getName` (which is unqualified and puts us in a
> > > better spot for the eventual support of c++ and nested types). and if
> > > that doesn't work fallback to the qualified name.
> > Sorry I meant `getTypedefName(Decl)`, which also tries to get the typedef
> > name for an anonymous tag.
> > The patch summary says "Anonymous enums that are typedef'd should take on
> > the name of the typedef." and I was a bit confused of the change.
> >
> > Now we have three (two fallbacks) for Enum:
> > 1. straightforward `Decl->getName()`, and if that's empty
> > 2. `getTypedefName(Decl)`, which calls `Decl->getTypedefNameForAnonDecl()`,
> > and if that fails,
> > 3. `Decl->printQualifiedName(OS)`
> >
> > My questions:
> > 1. Do we need all three? We should be able to get rid of at least one
> > right? The logic seems to be: if enum is named, use it. Otherwise get
> > typedef name if it's part of a typedef. And finally leave something like
> > `(anonymous)` if it's just an unattached anonymous enum.
> > 2. For the `printQualifiedName` path, isn't `Name` referencing a local
> > string inside the method?
> > 3. structs are also tags that can be anonymous and inside a typedef, do we
> > need similar changes there?
> >
> >
> 1. Yup that is exactly what I am trying to do here. If you can think of a way
> of removing a chunk then let's do it. The reason for the third fallback is to
> have a name for the container of an anonymous (without a typedef) top level
> enum (there is an example of that in the enum.c test case)
> 2. Not entirely sure what you mean. If you are referring to the fact that the
> backing storage for `Name` in this case is a local variable, this is fine
> because we copy the string into the `APISet` when constructing the record
> within this method a bit further down.
> 3. The struct case is a little different, It's not possible to have to have
> an anonymous struct without a typedef at the top level, whereas it is
> possible to do so with an enum (e.g. to introduce some constants). Currently
> for structs we only do `Decl->getName()` and falls back to
> `getTypedefName(Decl)`. If that fails we ignore the struct because it must be
> a nested anonymous struct (that logic is flawed and we should aim to support
> anonymous structs properly, but that would be a follow up patch to this one).
Ah I see. So before this patch we had `getQualifiedNameAsString` which would
always bypass the following `getTypedefName` even if it's an anonymous within a
typedef. This patch cleared out the logic and ordering.
> we copy the string into the APISet when constructing the record within this
> method a bit further down.
😅 right, that part was folded in the diff view and I totally forgot we had that
there...
LGTM. Thanks Daniel!
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D140010/new/
https://reviews.llvm.org/D140010
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
