[PATCH] D26567: Improve DWARF parsing speed by improving DWARFAbbreviationDeclaration

[Greg Clayton via llvm-commits]

clayborg added a comment.

Will update with changes.



================
Comment at: lib/DebugInfo/DWARF/DWARFAbbreviationDeclaration.cpp:178
+  size_t ByteSize = NumBytes;
+  if (NumAddrs > 0)
+    ByteSize += NumAddrs * U.getAddressByteSize();
----------------
aprantl wrote:
> `> 0` is redundant since NumAddrs is unsigned. I would either use `!= 0` of if (NumAddrs).
> 
> Actually, it looks like the entire condition is redundant. If NumAddrs is 0 then the next line will be a noop.
Will change.


================
Comment at: lib/DebugInfo/DWARF/DWARFAbbreviationDeclaration.cpp:192
+    return ByteSize;
+  return DWARFFormValue::getFixedByteSize(Form, &U);
+}
----------------
aprantl wrote:
> Maybe it's easier to read to have this as a ?: one-liner inlined the class declaration?
Not sure I follow? Do you want this?:

```
return ByteSize ? ByteSize : DWARFFormValue::getFixedByteSize(Form, &U);
```


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugInfoEntry.cpp:221
+    // Check if this attribute has a fixed byte size.
+    Optional<uint8_t> FixedSize = AttrSpec.getByteSize(U);
+    if (FixedSize) {
----------------
aprantl wrote:
> roll into the condition?
Will do.


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugInfoEntry.cpp:248
     dwarf::Attribute Attr, DWARFFormValue &FormValue) const {
-  if (!AbbrevDecl)
+  if (!AbbrevDecl || !U)
     return false;
----------------
aprantl wrote:
> I don't like that U can be null here but that's for another patch :-)
This will go away in my future DWARFDIE patch.


https://reviews.llvm.org/D26567