dang added inline comments.
================
Comment at: clang/lib/ExtractAPI/ExtractAPIVisitor.cpp:174-175
+ StringRef Name = Decl->getName();
if (Name.empty())
Name = getTypedefName(Decl);
+ if (Name.empty()) {
----------------
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).
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
