[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/Core/LVScope.cpp:2051
+  // Start traversing the scopes root and transform the element name.
+  TraverseScope(this);
+}
----------------
probinson wrote:
> I don't understand the reason to put the entire method implementation into a lambda, and then call it once.
`TraverseScope` is recursive.
```
  std::function<void(LVScope * Parent)> TraverseScope = [&](LVScope *Parent) {
    ...
    if (const LVScopes *Scopes = Parent->getScopes())
      for (LVScope *Scope : *Scopes) {
        Scope->setInnerComponent();
        TraverseScope(Scope);
      }
    ...
  };
```
It traverses the parent scopes.


================
Comment at: llvm/lib/DebugInfo/LogicalView/LVReaderHandler.cpp:53
+      if (Obj.isCOFF()) {
+        COFFObjectFile *COFF = dyn_cast<COFFObjectFile>(&Obj);
+        return new LVCodeViewReader(Filename, FileFormatName, *COFF, W,
----------------
probinson wrote:
> `dyn_cast` is used when the result might not be of the specified type, in which case you get a null pointer.  I think `cast` is the right thing to use here.
You are correct. Changed to `cast`.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewReader.cpp:59
+
+std::string LVCodeViewReader::getSymbolKindName(SymbolKind Kind) {
+  switch (Kind) {
----------------
probinson wrote:
> It looks like this is almost exactly getSymbolKindName from SymbolDumper.cpp.  I don't see an easy way to make use of that function, though.
> 
> Return type should be a StringRef, no need to make a copy of the string literal.
> 
You are correct. I have add it to the list of future work.
Easy allow to access to `getSymbolKindName` and `formatRegisterId`.
Changed the return type to `StringRef`.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewReader.cpp:68
+  }
+}
+
----------------
probinson wrote:
> Might need an `llvm_unreachable` here to avoid warnings about falling off the end of the function without a return value.
Added
```
  llvm_unreachable("Unknown SymbolKind::Kind");
```



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

https://reviews.llvm.org/D125784