This patch generalizes the ParsedDWARFTypeAttributes struct to be able to share with other DWARF parsers other than clang. This patch also reduces the memory footprint of the struct by using bit flags instead of individual booleans.
Diff Detail
Event Timeline
I'm not sure if this falls into NFC category since I'm changing how flags are stored.
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)?
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...
lldb/source/Plugins/SymbolFile/DWARF/DWARFAttribute.h | ||
---|---|---|
83 | 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. | |
123 | inline on a method declared inside the class is redundant |
lldb/source/Plugins/SymbolFile/DWARF/DWARFAttribute.h | ||
---|---|---|
86 | I forgot to add the rest of the documentation. Leaving a note here for myself |
I also echo Pavel's question, why not in DWARFASTParser?
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp | ||
---|---|---|
1139–1140 | 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. | |
1492 | if we are going to have getter abstraction why not have a setIsForwardDeclaration() or something similar. |
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.
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.
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp | ||
---|---|---|
1139–1140 | I think the abstraction of the external attribute is valid since storage is Clang specific StorageClass. | |
1492 | 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. | |
lldb/source/Plugins/SymbolFile/DWARF/DWARFAttribute.h | ||
86 | Added documentation. |
I can't help the feeling that the boilerplate introduced by the enum compression is just not worth the benefit it brings, but otherwise this looks ok apart from the two inline comments.
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParser.h | ||
---|---|---|
90 | What's up with this? I would hope that clang-format is able to reasonably format a simple enum declaration like this. | |
lldb/source/Plugins/SymbolFile/DWARF/DWARFAttribute.h | ||
12–16 | revert this. |
What's up with this?
I would hope that clang-format is able to reasonably format a simple enum declaration like this.