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

[Greg Clayton via llvm-commits]

clayborg added a comment.

Will update according to comments.



================
Comment at: lib/DebugInfo/DWARF/DWARFFormValue.cpp:57
 
+Optional<uint8_t> DWARFFormValue::getFixedByteSize(dwarf::Form form,
+                                                   const DWARFUnit *U) {
----------------
aprantl wrote:
> Would it make sense / be possible to encode this tabular data (at least for the majority of values) in the macros in Dwarf.def?
Not sure how you would do this since many don't have a byte size. Zero is a valid DW_FORM size, so you can't use zero for items that don't have byte sizes. I wouldn't want to have a signed integer as the size either where -1 would indicate that there is no size. So since there is no easy way to encode an optional value with no value into the table, I would vote against this. Agains, this is the only function that needs to be updated.


================
Comment at: lib/DebugInfo/DWARF/DWARFFormValue.cpp:364
 
-bool
-DWARFFormValue::skipValue(DataExtractor debug_info_data, uint32_t* offset_ptr,
-                          const DWARFUnit *U) const {
-  return DWARFFormValue::skipValue(Form, debug_info_data, offset_ptr, U);
-}
-
-bool
-DWARFFormValue::skipValue(dwarf::Form form, DataExtractor debug_info_data,
-                          uint32_t *offset_ptr, const DWARFUnit *cu) {
-  return skipValue(form, debug_info_data, offset_ptr, cu->getVersion(),
-                   cu->getAddressByteSize());
-}
-bool 
-DWARFFormValue::skipValue(dwarf::Form form, DataExtractor debug_info_data,
-                          uint32_t *offset_ptr, uint16_t Version,
-                          uint8_t AddrSize) {
+bool skipVariableLengthValue(dwarf::Form form,
+                             const DataExtractor &debug_info_data,
----------------
aprantl wrote:
> I know it was like this in the original source, but while you are editing it, we could use the opportunity fix the variable names to start with uppercase (http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly). Up to you.
I desire to keep this patch small so the extra diffs don't cloud our ability to see changes. I will do a follow up patch where I do nothing but case changing. 


================
Comment at: lib/DebugInfo/DWARF/DWARFFormValue.cpp:424
+      } else {
+        assert(!"only variable length Form can be passed to this function");
+      }
----------------
aprantl wrote:
> Should this really be an assertion, in the sense of that it indicates a programming error in the caller? Or can this situation be triggered by malformed DWARF input and should we return an Error instead?
This function is internal to this file and is designed to only be called for variable length forms. The callers of this function both do:

```
  if (Optional<uint8_t> FixedByteSize = getFixedByteSize(form, U)) {
    *offset_ptr += *FixedByteSize;
    return true;
  }
  return skipVariableLengthValue(...);
```

I turned this into an llvm_unreachable().


https://reviews.llvm.org/D26514