ljmf00 accepted this revision. ljmf00 added a comment. This revision is now accepted and ready to land.
In D114746#3160908 <https://reviews.llvm.org/D114746#3160908>, @labath wrote: > I don't think we should be putting this into the DWARFAttribute file. It is > substantially higher-level than the rest of the file, and the DWARFAttribute > file is destined to be merged with the llvm implementation at some point. Is > there a reason for not putting it into `DWARFASTParser` (like the other > patch)? I changed to DWARFASTParser. There was no particular reason other than naming, but since I lack some knowledge about the overall infrastructure I was not sure. > In D114746#3160331 <https://reviews.llvm.org/D114746#3160331>, @ljmf00 wrote: > >> I'm not sure if this falls into NFC category since I'm changing how flags >> are stored. > > There's no formal definition of "NFC", so it does not really matter. Oh ok. I read it on the Developers Policy glossary, so I was not sure. Thanks for the clarification. >> This patch also reduces the memory footprint of the struct by using bit >> flags instead of individual booleans. > > The class was never meant to be stored anywhere else except the stack of the > function doing the parsing, so it's not really necessary to optimize it that > much. But since, you've already done it, I suppose it can stay... Well, I didn't make any performance tests but it surely can improve memory usage. I guess the compiler can also be smart if the memory is only on the stack. ================ Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1140 : containing_decl_ctx, - GetOwningClangModule(die), name, clang_type, attrs.storage, - attrs.is_inline); + GetOwningClangModule(die), name, clang_type, attrs.is_external() ? clang::SC_Extern : clang::SC_None, + attrs.is_inline()); ---------------- shafik wrote: > Is there a reason not to use an attribute `storage` like before? This feels > like we are leaking out of the abstraction where before we were not. I think the abstraction of the external attribute is valid since `storage` is Clang specific StorageClass. ================ Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1493 // and the byte size is zero. - attrs.is_forward_declaration = true; + attrs.attr_flags |= eDWARFAttributeIsForwardDecl; } ---------------- shafik wrote: > if we are going to have getter abstraction why not have a > `setIsForwardDeclaration()` or something similar. Yeah, I can see the improvement, I added setters too. I didn't add them since the set was only used as a statement rather than an expression. ================ Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFAttribute.h:86 + /// Whether it is an artificially generated symbol. + eDWARFAttributeIsArtificial = (1u << 0), + eDWARFAttributeIsExplicit = (1u << 1), ---------------- ljmf00 wrote: > I forgot to add the rest of the documentation. Leaving a note here for myself Added documentation. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114746/new/ https://reviews.llvm.org/D114746 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
