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

[Carlos Alberto Enciso via Phabricator via llvm-commits]

CarlosAlbertoEnciso added inline comments.


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


================
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) {
----------------
psamolysov wrote:
> 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`.
Changed to use `typename`.


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


================
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) << ", "
----------------
psamolysov wrote:
> I believe this should work too.
Nice change.


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


================
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);
----------------
psamolysov wrote:
> 
Nice change.


================
Comment at: llvm/unittests/DebugInfo/LogicalView/WarningInternalTest.cpp:92
+    T *Element = new T();
+    EXPECT_NE(Element, nullptr);
+    return Element;
----------------
psamolysov wrote:
> 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.
Changed to use the non-throwing overload.


================
Comment at: llvm/unittests/DebugInfo/LogicalView/WarningInternalTest.cpp:176
+  Element->setType(Type);
+  EXPECT_STREQ(Element->getName().data(), Name.data());
+  EXPECT_EQ(Element->getOffset(), Offset);
----------------
psamolysov wrote:
> 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.
Changed to EXPECT_EQ(Element->getName(), Name);


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125781