[Lldb-commits] [PATCH] D40467: DWZ 04/12: Separate Offset also into FileOffset

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

Please undo all renaming from offset to file_offset. The "offset" you get from a  DIE should always be the absolute .debug_info offset. No need to say file_offset. Patch will be easier to read after spurious renames are removed.



================
Comment at: include/lldb/Expression/DWARFExpression.h:221-222
   void CopyOpcodeData(lldb::ModuleSP module_sp, const DataExtractor &data,
-                      lldb::offset_t data_offset, lldb::offset_t data_length);
+                      lldb::offset_t data_file_offset,
+                      lldb::offset_t data_length);
 
----------------
don't rename, revert


================
Comment at: source/Expression/DWARFExpression.cpp:92-94
+                                     lldb::offset_t data_file_offset,
                                      lldb::offset_t data_length) {
+  const uint8_t *bytes = data.PeekData(data_file_offset, data_length);
----------------
don't rename, revert


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFAttribute.cpp:54
   form_value.SetForm(FormAtIndex(i));
-  lldb::offset_t offset = DIEOffsetAtIndex(i);
+  lldb::offset_t file_offset = cu->ThisCUUniqToFileOffset(DIEOffsetAtIndex(i));
   return form_value.ExtractValue(
----------------
Why is this not just done correctly inside DIEOffsetAtIndex? No need to rename the variable.


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFAttribute.cpp:56
   return form_value.ExtractValue(
-      cu->GetSymbolFileDWARF()->get_debug_info_data(), &offset);
+      cu->GetSymbolFileDWARF()->get_debug_info_data(), &file_offset);
 }
----------------
don't rename.


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.h:207-226
+  dw_offset_t GetFileOffset() const { return m_data->m_file_offset; }
+  dw_offset_t FileOffsetToUniqOffset(dw_offset_t file) const {
+    return ThisCUFileOffsetToUniq(file);
+  }
+  static dw_offset_t ThisCUFileOffsetToUniq_nocheck(dw_offset_t file) {
+    return file;
+  }
----------------
Not a fan of these names. Can't these just be something like:

```
dw_offset_t GeCUtRelativeOffset(); // CU relative offset
dw_offset_t GetOffset(); // Always absolute .debug_info offset
```

ThisCUUniqToFileOffset seems a bit unclear.


https://reviews.llvm.org/D40467