[PATCH] D140010: [clang][ExtractAPI] Fix naming of typedef'd anonymous enums

[Zixu Wang via Phabricator via cfe-commits]

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