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

[Carlos Alberto Enciso via Phabricator via llvm-commits]

CarlosAlbertoEnciso added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Readers/LVCodeViewReader.h:177
+  /// This function taken from (COFFDumper.cpp).
+  /// It can be moved to the COFF library.
+  Error initializeFileAndStringTables(BinaryStreamReader &Reader);
----------------
probinson wrote:
> Use TODO: or FIXME: comments for things that should be done later.
Added `TODO:` and put it in the list for future work.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVElement.cpp:245
 void LVElement::generateName(std::string &Prefix) const {
+  // CodeView elements that are not referenced and not finalized.
   LVScope *Scope = getParentScope();
----------------
probinson wrote:
> I'm not sure what this means.  As written, it would describe a collection of elements that aren't referenced or finalized, but there's no such collection here.
> If you remove "that", I understand it to mean this method doesn't do anything with those CodeView elements.  
> If the phrase means something else, I'd need an explanation.
This method is called for all logical elements regardless of the debug info format. It generates a name for unnamed elements.
Removed the comment as it makes no sense.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVReader.cpp:184
+  // multiple compile units. Until a proper offset calculation, check only
+  // in the current compile unit.
   if (CompileUnits.size()) {
----------------
probinson wrote:
> This sounds like a FIXME/TODO kind of comment.
You are correct. Already added to list for future work. Added `TODO:`.
```
  // TODO: The current CodeView Reader implementation does not have support
  // for multiple compile units. Until we have a proper offset calculation,
  // check only in the current compile unit.
```


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewReader.cpp:514
+      }
+      break;
+    }
----------------
probinson wrote:
> 
Good point. Added your suggested comment.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewReader.cpp:1019
+        // The CodeView compile unit containing the global symbols does not
+        // have a name; use the  and it is marked as such. Just append the
+        // '_global' string.
----------------
probinson wrote:
> "use the  and it is marked as such."  Could you rephrase?
Rephrased as:
```
        // The CodeView compile unit containing the global symbols does not
        // have a name; generate one using its parent name (object filename)
        // follow by the '_global' string.
```


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewVisitor.cpp:766
+
+  CurrentElement = LogicalVisitor->createElement(Kind);
+  if (!CurrentElement) {
----------------
probinson wrote:
> 
Despite that `CurrentElement` it is used only in this method, keep it in the `LogicalVisitor`.


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

https://reviews.llvm.org/D125784