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

[Adrian Prantl via llvm-commits]

aprantl added inline comments.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFFormValue.h:94
+  /// returned.
+  /// @param Form The DWARF form to get the fixed byte size for
+  /// @param U The DWARFUnit that can be used to help determine the byte size.
----------------
Nitpick: \param
(http://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments)


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFFormValue.h:101
+                                            const DWARFUnit *U = nullptr);
+  // Get the fixed byte size for a given form. If the form is always encoded
+  // using a variable length storage format (ULEB/SLEB numbers or blocks) then
----------------
///


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFFormValue.h:122
                  const DWARFUnit *U) const;
   static bool skipValue(dwarf::Form form, DataExtractor debug_info_data,
                         uint32_t *offset_ptr, const DWARFUnit *u);
----------------
Could you use the opportunity and document what the bool return value indicates? It's not clear to me from reading the function.


================
Comment at: lib/DebugInfo/DWARF/DWARFFormValue.cpp:57
 
+Optional<uint8_t> DWARFFormValue::getFixedByteSize(dwarf::Form form,
+                                                   const DWARFUnit *U) {
----------------
Would it make sense / be possible to encode this tabular data (at least for the majority of values) in the macros in Dwarf.def?


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


================
Comment at: lib/DebugInfo/DWARF/DWARFFormValue.cpp:424
+      } else {
+        assert(!"only variable length Form can be passed to this function");
+      }
----------------
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?


================
Comment at: tools/llvm-dwp/llvm-dwp.cpp:138
+  uint64_t Length = InfoData.getU32(&Offset);
+  if (Length == 0xffffffffU) {
+    Dwarf32 = false;
----------------
Comment what this magic indicator does?


https://reviews.llvm.org/D26514