This is an archive of the discontinued LLVM Phabricator instance.

Modify TypePrinter to differentiate between anonymous struct and unnamed struct
ClosedPublic

Authored by shafik on Feb 16 2021, 11:49 AM.

Details

Summary

Currently TypePrinter lumps anonymous classes and unnamed classes in one group "anonymous" this is not correct and can be confusing in some contexts. e.g.

LLDB SBType provides an IsAnonymousType() function but when displaying the type we see conflicting information, given:

  struct A { 
   struct { int x; }; // A
   struct { int y; } B;
};

int main() {
   A a1; 
  //...
}

we get the following in LLDB:

(lldb) script var = lldb.frame.FindVariable("a1")
(lldb) script print(var.GetChildAtIndex(0).GetType().IsAnonymousType())
True
(lldb) script print(var.GetChildAtIndex(1).GetType().IsAnonymousType())
False
(lldb) script print(var.GetChildAtIndex(0).GetType().GetName())
A::(anonymous struct)
(lldb) script print(var.GetChildAtIndex(1).GetType().GetName())
A::(anonymous struct)

Diff Detail

Event Timeline

shafik created this revision.Feb 16 2021, 11:49 AM
shafik requested review of this revision.Feb 16 2021, 11:49 AM

Note: I am not fixing how we treat anonymous and unnamed enums, I could not find a way to figure out if an enum was anonymous or not.

Thank you for working on this! In general, I like the change.

clang/lib/AST/TypePrinter.cpp
1308

I think EnumDecl should probably be unnamed rather than anonymous because there's no term of art for an anonymous enumeration (there's only anonymous structures and anonymous unions). WDYT?

LGTM.

Regarding the LLDB example: Given that the LLDB API is in theory not bound to the semantics a specific language, I think one can argue that IsAnonymousType could also return true for unnamed classes. The use case that triggered this whole discussion was someone who wanted to know whether the type has a name, so calling this function seems logical. Given that we don't really have a valid type name for unnamed classes, we might want to change the implementation of IsAnonymousType to also return true for unnamed classes. But all that is out of scope for this patch which is about fixing the type printer in Clang. Just bringing this up as it's the example in the patch description.

shafik updated this revision to Diff 324431.Feb 17 2021, 2:32 PM
  • Went with unnamed enums Vs anonymous enums
shafik marked an inline comment as done.Feb 17 2021, 2:33 PM
shafik added inline comments.
clang/lib/AST/TypePrinter.cpp
1308

I thought I saw that wording but when I looked back I realize you are correct.

This revision is now accepted and ready to land.Feb 18 2021, 4:46 AM
This revision was automatically updated to reflect the committed changes.
shafik marked an inline comment as done.
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptFeb 18 2021, 5:45 PM

Reverted the changes because I missed the clangd test suite and don't know how long it will take to fix.

shafik updated this revision to Diff 325100.Feb 19 2021, 3:24 PM

Fixing tests that I missed before.