[PATCH] D125778: [llvm-dva] 03 - Logical elements

[Carlos Alberto Enciso via Phabricator via llvm-commits]

CarlosAlbertoEnciso added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVObject.h:20
+#include "llvm/DebugInfo/CodeView/TypeIndex.h"
 #include "llvm/DebugInfo/LogicalView/Core/LVSupport.h"
+#include <list>
----------------
psamolysov wrote:
> Could you add `#include <limits>` for `std::numeric_limits<uint64_t>`?
Added the header file.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVReader.h:21
+#include "llvm/Support/ToolOutputFile.h"
+
+namespace llvm {
----------------
psamolysov wrote:
> What about `#include <map>` to make the header more self-contained?
Added the header file.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h:52
 
+using LVOffsetElementMap = std::map<LVOffset, LVElement *>;
+
----------------
psamolysov wrote:
> Let's add `#include <map>` to make the header more self-contained and independent of the other included headers.
Added header file.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h:304
+  // Names (files and directories) used by the Compile Unit.
+  std::vector<size_t> Filenames;
+
----------------
psamolysov wrote:
> Is this a vector of indices?
Yes. The vector contains indices into the String Pool table.
`Filenames.push_back(getStringPool().getIndex(Name));`


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVSupport.h:33
+public:
+  typedef typename SmallVector<T, N>::iterator iterator;
+  LVAutoSmallVector() : SmallVector<T, N>::SmallVector() {}
----------------
psamolysov wrote:
> 
Replaced.


================
Comment at: llvm/lib/DebugInfo/LogicalView/CMakeLists.txt:33
   "${LLVM_MAIN_INCLUDE_DIR}/llvm/DebugInfo/LogicalView/Core"
+  "${LLVM_MAIN_INCLUDE_DIR}/llvm/DebugInfo/LogicalView/Readers"
   )
----------------
psamolysov wrote:
> I see no headers in the `Readers` path in the patch. Should the line be moved in a next patch in the series or `LVReader.h` moved in the `Readers` directory?
Removed the line `"${LLVM_MAIN_INCLUDE_DIR}/llvm/DebugInfo/LogicalView/Readers"` and moved to next patch.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVElement.cpp:431
+void LVElement::printFileIndex(raw_ostream &OS, bool Full) const {
+  if (options().getPrintFormatting() && options().getAttributeAnySource() &&
+      getFilenameIndex()) {
----------------
psamolysov wrote:
> Early return if the inverted condition takes place is more preferable by the coding style.
I see your point.

```
if (!(options().getPrintFormatting() && options().getAttributeAnySource() &&
      getFilenameIndex()))
  return;
```
But with the inverted condition, it seems the statetment gets obscure.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVLine.cpp:111
+  if (options().getAttributeQualifier()) {
+    // The qualifier include the states information and the source filename
+    // that contains the line element.
----------------
psamolysov wrote:
> 
Corrected spelling.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVReader.cpp:28
+//===----------------------------------------------------------------------===//
+// Class to represent an split context.
+//===----------------------------------------------------------------------===//
----------------
psamolysov wrote:
> 
Corrected spelling.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVReader.cpp:90
+
+    // Return error if enable to create a split context location.
+    if (Error Err = SplitContext.createSplitFolder(SplitFolder))
----------------
psamolysov wrote:
> if unable?
Correct. Fixed the spelling.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVReader.cpp:115
+// information contained in the binary file specified in the command line.
+// This is the main entry point for the Reader and perform the following
+// steps:
----------------
psamolysov wrote:
> 
Corrected spelling.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:110-111
+void LVScope::addElement(LVLine *Line) {
+  assert(!Line->getParent() && "Line already inserted");
+  assert(Line && "Invalid line.");
+  if (!Lines)
----------------
psamolysov wrote:
> It definitely makes sense to swap these two lines.
Very good catch. Fixed.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:134-135
+void LVScope::addElement(LVScope *Scope) {
+  assert(!Scope->getParent() && "Scope already inserted");
+  assert(Scope && "Invalid scope.");
+  if (!Scopes)
----------------
psamolysov wrote:
> It definitely makes sense to swap these two lines.
Very good catch. Fixed.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:161-162
+void LVScope::addElement(LVSymbol *Symbol) {
+  assert(!Symbol->getParent() && "Symbol already inserted");
+  assert(Symbol && "Invalid symbol.");
+  if (!Symbols)
----------------
psamolysov wrote:
> It definitely makes sense to swap these two lines.
Very good catch. Fixed.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:188-189
+void LVScope::addElement(LVType *Type) {
+  assert(!Type->getParent() && "Type already inserted");
+  assert(Type && "Invalid type.");
+  if (!Types)
----------------
psamolysov wrote:
> It definitely makes sense to swap these two lines.
Very good catch. Fixed.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:874
+// Increase the added element counters, for printing summary.
+// Notify the Reader if element comparison.
+void LVScopeCompileUnit::addedElement(LVLine *Line) { increment(Line); }
----------------
psamolysov wrote:
> There is no notifications are sent below. Should this comment line be added to a next patch in the series?
Correct. That comment should be in the `compare elements` patch.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:1161
+    LVScope *Reference = getReference();
+    if (Reference->getIsExternal()) {
+      Reference->resetIsExternal();
----------------
psamolysov wrote:
> 
Added the extra check.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:1247
+  if (const LVSymbols *Symbols = getSymbols()) {
+    AddComma = false;
+    for (LVSymbol *Symbol : *Symbols)
----------------
psamolysov wrote:
> `AddComma` has already been initialized, this line looks as not required.
Moved the definition/initialization inside the if.
```
  if (const LVSymbols *Symbols = getSymbols()) {
    bool AddComma = false;
```



================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVType.cpp:254
+  //   "std::type<int,std::less<int>,std::allocator<int>,false>"
+  // Instead of the the incomplete names:
+  //   "type<float,less,allocator,false>"
----------------
psamolysov wrote:
> 
Removed the extra `the`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125778