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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

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)?

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.

> 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...



================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFAttribute.h:83
 
+FLAGS_ENUM(DWARFAttributeFlags)
+{
----------------
This macro was created specifically for using in lldb public api. Elsewhere you can just use the standard llvm solution (search for LLVM_MARK_AS_BITMASK_ENUM).

Since this attribute is not supposed to be visible/useful outside the `ParsedDWARFTypeAttributes` class, I think it'd be better to declare it there.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFAttribute.h:123
+
+  inline bool is_artificial() const {
+    return attr_flags & eDWARFAttributeIsArtificial;
----------------
`inline` on a method declared inside the class is redundant


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114746/new/

https://reviews.llvm.org/D114746