[PATCH] D125783: [llvm-debuginfo-analyzer] 08 - ELF Reader

[Carlos Alberto Enciso via Phabricator via llvm-commits]

CarlosAlbertoEnciso added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Readers/LVBinaryReader.h:152
+  LVBinaryReader &operator=(const LVBinaryReader &) = delete;
+  ~LVBinaryReader() {
+    // Delete the containers created by 'createInstructions'.
----------------
probinson wrote:
> probinson wrote:
> > Should the `LVBinaryReader` destructor be virtual?
> You might think about whether it makes sense to put the destructor into the .cpp file so the allocations and deallocations are at least somewhat near each other.
You are correct. Marked `LVBinaryReader` destructor as `virtual`.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Readers/LVELFReader.h:111
+
+  // Get the location information for DW_AT_data_member_location.
+  void getLocationMember(dwarf::Attribute Attr, const DWARFFormValue &FormValue,
----------------
probinson wrote:
> It seems strange to have "get" methods that don't return anything.  Are there better names?
Renamed as `processLocationMember`.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFExpression.cpp:231
                                   const uint64_t Operands[2],
                                   const MCRegisterInfo *MRI, bool isEH) {
   if (!MRI)
----------------
probinson wrote:
> Parameters should be re-aligned to conform to formatting standard.
Applied correct alignment.


================
Comment at: llvm/lib/DebugInfo/LogicalView/LVReaderHandler.cpp:94
+                             "Binary object format in '%s' is not supported.",
+                             TheFilename.c_str());
+  }
----------------
probinson wrote:
> `StringRef` has a `str()` method that returns `std::string` so you can eliminate the local variable `TheFilename`.
Nice suggestion. Removed `TheFilename`.


================
Comment at: llvm/lib/DebugInfo/LogicalView/LVReaderHandler.cpp:107
+  if (BuffOrErr.getError()) {
+    std::string TheFilename(ConvertedPath);
+    return createStringError(errc::bad_file_descriptor,
----------------
probinson wrote:
> ConvertedPath is already a std::string, no need to make a copy.
Removed `TheFilename` and changed to

```
    return createStringError(errc::bad_file_descriptor,
                             "File '%s' does not exist.",
                             ConvertedPath.c_str());
```



================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVELFReader.cpp:795
+                             "Could not create DWARF information: %s",
+                             TheFilename.c_str());
+  }
----------------
probinson wrote:
> If `getFilename()` returns a StringRef, you can do `getFilename().str().c_str()` and then don't need TheFilename.
Nice suggestion. Removed `TheFilename`.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVELFReader.cpp:862
+    // same, do not perform any adjustment.
+    auto DeductIncrementFileIndex = [&]() -> bool {
+      if (CU->getVersion() < 5)
----------------
probinson wrote:
> "Deduct" means "subtract" which is not what you mean here.
Renamed as `DeduceIncrementFileIndex`.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVELFReader.cpp:958
+// Get the location information for the associated attribute.
+void LVELFReader::getLocationList(dwarf::Attribute Attr,
+                                  const DWARFFormValue &FormValue,
----------------
probinson wrote:
> This method doesn't "get" information in the usual sense (return it to the caller).
> Maybe `processLocationList` ?  Consistent with the lambda names in the implementation.
Renamed as `processLocationList`.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVELFReader.cpp:1049
+
+void LVELFReader::getLocationMember(dwarf::Attribute Attr,
+                                    const DWARFFormValue &FormValue,
----------------
probinson wrote:
> Maybe `processMemberLocation` ?
Renamed as `processMemberLocation`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125783/new/

https://reviews.llvm.org/D125783