[PATCH] D125784: [llvm-debuginfo-analyzer] 09 - CodeView Reader

[Carlos Alberto Enciso via Phabricator via llvm-commits]

CarlosAlbertoEnciso added inline comments.


================
Comment at: llvm/lib/DebugInfo/LogicalView/LVReaderHandler.cpp:192
+          "Binary object format in '%s' does not have debug info.",
+          TheFilename.c_str());
+    }
----------------
probinson wrote:
> No need for local variable `TheFilename` .
Nice suggestion. Removed `TheFilename`.


================
Comment at: llvm/lib/DebugInfo/LogicalView/LVReaderHandler.cpp:272
+    return createStringError(errorToErrorCode(std::move(Err)), "%s",
+                             TheFilename.c_str());
+  }
----------------
probinson wrote:
> No need to construct `TheFilename` explicitly.
Nice suggestion. Removed `TheFilename`.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewReader.cpp:400
+                               "File '%s' does not exist.",
+                               TheFilename.c_str());
+    }
----------------
probinson wrote:
> No need to construct `TheFilename` explicitly.
Nice suggestion. Removed `TheFilename`.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewReader.cpp:412
+    return createStringError(errorToErrorCode(std::move(Err)), "%s",
+                             TheFilename.c_str());
+  }
----------------
probinson wrote:
> `SmallString` has a direct method for getting a C string, much more efficient than going through std::string (no copying).
Nice suggestion. Removed `TheFilename`.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewReader.cpp:467
+                             "Binary object format in '%s' is not supported.",
+                             TheFilename.c_str());
+  }
----------------
probinson wrote:
> `SmallString` has a more efficient direct conversion to C string.
Nice suggestion. Removed the `TheFilename`.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewReader.cpp:474
+    return createStringError(errc::not_supported, "'%s' is not a COFF object.",
+                             TheFilename.c_str());
+  }
----------------
probinson wrote:
> 
Nice suggestion. Removed `TheFilename`.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewReader.cpp:929
+    if (BuffOrErr.getError()) {
+      std::string TheFilename(ExePath);
+      return createStringError(errc::bad_file_descriptor,
----------------
probinson wrote:
> In this context I think `ExePath` is the class member? which is already a std::string.
Yes you are correct. Removed `TheFilename`.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewReader.cpp:944
+    if (errorToErrorCode(BinOrErr.takeError())) {
+      std::string TheFilename(ExePath);
+      return createStringError(errc::not_supported,
----------------
probinson wrote:
> I think `ExePath` is already a std::string.
Yes you are correct. Removed `TheFilename`.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewVisitor.cpp:441
+      std::string TheComponent(Component);
+      dbgs() << formatv("'{0}'\n", TheComponent.c_str());
+    }
----------------
probinson wrote:
> 
Nice suggestion. Removed `TheComponent`.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewVisitor.cpp:482
+    std::string TheScopedName(ScopedName);
+    dbgs() << formatv("ScopedName: '{0}'\n", TheScopedName.c_str());
+  });
----------------
probinson wrote:
> 
Nice suggestion. Removed `TheScopeName`.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewVisitor.cpp:3375
+    std::string TheName(Name);
+    OS << format("%20s", TheName.c_str());
+    NewLine();
----------------
probinson wrote:
> Unless format() understands StringRef directly?  I'm not sure.
>From my understanding `format` needs C-type string.
Nice suggestion. Removed `TheName`.


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

https://reviews.llvm.org/D125784