[Lldb-commits] [PATCH] D114746: [lldb] Generalize ParsedDWARFTypeAttributes

[Luís Ferreira via Phabricator via lldb-commits]

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