[PATCH] D125781: [llvm-dva] 06 - Warning and internal options

[Pavel Samolysov via Phabricator via llvm-commits]

psamolysov added a comment.

Some my comments repeat comments to previous patches, sorry if it looks noisy. Could you check the `{A, B}` replacement of the `std::make_pair(A, B)`? If it works, there is a couple of places where `std::make_pair` is used and they can be replaced with `{A, B}` too.



================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h:57
 
+using LVOffsetList = std::list<LVOffset>;
 using LVOffsetElementMap = std::map<LVOffset, LVElement *>;
----------------
What about to add `#include <list>` to make the header more self-contained?


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVSupport.h:155
+// Add an item to a map with second being a list.
+template <typename MapType, class ListType, class KeyType, class ValueType>
+void addItem(MapType *Map, KeyType Key, ValueType Value) {
----------------
This is a bit inconsistent declaration: `MapType` is declared with `typename` while all the other types are declared with `class`. Something one should be used: either `typename` or `class`.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVSupport.h:162
+  else {
+    List = new ListType();
+    Map->insert(std::make_pair(Key, List));
----------------
I'm sorry but I cannot find where the `List` is deleted. So, I get the `Map` takes ownership of the `List` but there is no destructor of function where the `Map`'s entries (of the `DebugTags` map, for example) is deleted.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVSupport.h:163
+    List = new ListType();
+    Map->insert(std::make_pair(Key, List));
+  }
----------------
I believe this should work too.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVReader.cpp:41
+    if (Iter == Integrity.end())
+      Integrity.insert(std::make_pair(Element, Scope));
+    else
----------------
I believe this should work.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVReader.cpp:44
+      // We found a duplicated.
+      Duplicate.push_back(LVDuplicateEntry(Element, Scope, Iter->second));
+  };
----------------
To create an `LVDuplicateEntry` in place.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVReader.cpp:72
+
+    auto printIndex = [&](unsigned Index) {
+      if (Index)
----------------
If `format` is the `llvm::format` function, the lambda captures nothing.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:1118
+// Record scopes with invalid ranges.
+void LVScopeCompileUnit::addInvalidRange(LVLocation *Location) {
+  LVScope *Scope = Location->getParentScope();
----------------
The code is mostly identical to `LVScopeCompileUnit::addInvalidLocation(LVLocation *Location)`, it may make sense to extract the body in a `static` free function and pass into it an additional parameter: `LVOffsetLocationsMap` which is either `&InvalidLocations` or `&InvalidRanges`.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:1196
+    printHeader(Header);
+    for (const LVOffsetLocationsMap::value_type &Entry : Map) {
+      printElement(WarningOffsets, Entry.first);
----------------
I believe this should work too.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:1207
+    printHeader("Unsupported DWARF Tags");
+    for (const LVTagOffsetsMap::value_type &Entry : DebugTags) {
+      OS << format("\n0x%02x", (unsigned)Entry.first) << ", "
----------------
I believe this should work too.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:1220
+    printHeader("Symbols Invalid Coverages");
+    for (const LVOffsetSymbolMap::value_type &Entry : InvalidCoverages) {
+      // Symbol basic information.
----------------
I believe this should work too.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:1233
+    printHeader("Lines Zero References");
+    for (const LVOffsetLinesMap::value_type &Entry : LinesZero) {
+      printElement(WarningOffsets, Entry.first);
----------------



================
Comment at: llvm/unittests/DebugInfo/LogicalView/WarningInternalTest.cpp:92
+    T *Element = new T();
+    EXPECT_NE(Element, nullptr);
+    return Element;
----------------
A `std::bad_alloc` will be thrown if the test is out of memory. It makes sense to wrap the line in the `try-catch` block or use the non-throwing overload of the `new` operator.


================
Comment at: llvm/unittests/DebugInfo/LogicalView/WarningInternalTest.cpp:176
+  Element->setType(Type);
+  EXPECT_STREQ(Element->getName().data(), Name.data());
+  EXPECT_EQ(Element->getOffset(), Offset);
----------------
There is an overload of the `operator==` for two `StringRef`s, so there is no needs in `EXPECT_STREQ`, moreover the `data()` method of `StringRef` doesn't guarantee that the returned string will be null terminated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125781