[PATCH] D26514: Get rid of DWARFFormValue::getFixedFormSizes(uint8_t AddrSize, uint16_t Version).

[David Blaikie via llvm-commits]

dblaikie added a comment.

Just a few drive-by comments.



================
Comment at: include/llvm/DebugInfo/DWARF/DWARFFormValue.h:115
+  /// @param AddrSize size of an address in bytes.
+  /// @param Dwarf32 true for 32 bit DWARF, false for 64 bit DWARF
+  /// @return Optional<uint8_t> value with the fixed byte size or None if
----------------
Might be worth having an enum for this, for readability.


================
Comment at: lib/DebugInfo/DWARF/DWARFFormValue.cpp:130
+  default:
+    assert(!"Handle this form in this switch statement");
+    return None;
----------------
prefer llvm_unreachable over assert(false) (& there's no need for the return afterwards)


================
Comment at: lib/DebugInfo/DWARF/DWARFFormValue.cpp:463-464
+                               uint32_t *offset_ptr, const DWARFUnit *U) {
+  Optional<uint8_t> FixedByteSize = getFixedByteSize(form, U);
+  if (FixedByteSize) {
+    *offset_ptr += *FixedByteSize;
----------------
It's pretty common in LLVM to roll a variable declaration into the condition where possible:

  if (Optional<uint8_t> FixedByteSize = getFixedByteSize(form, U)) {
    ...
  }

(similar feedback elsewhere)


https://reviews.llvm.org/D26514