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

[Carlos Alberto Enciso via Phabricator via llvm-commits]

CarlosAlbertoEnciso added inline comments.


================
Comment at: llvm/unittests/DebugInfo/LogicalView/CodeViewReaderTest.cpp:37-39
+  EXPECT_NE(Parent, nullptr);
+  const LVScopes *Scopes = Parent->getScopes();
+  EXPECT_NE(Scopes, nullptr);
----------------
probinson wrote:
> 
`ASSERT_NE` can be used only inside functions that don't return a value.
 
See https://google.github.io/googletest/advanced.html#assertion-placement

> The one constraint is that assertions that generate a fatal failure (FAIL* and ASSERT_*) can only be used in void-returning functions.


================
Comment at: llvm/unittests/DebugInfo/LogicalView/CodeViewReaderTest.cpp:84-85
+  const LVLocations *Ranges = Function->getRanges();
+  EXPECT_NE(Ranges, nullptr);
+  ASSERT_EQ(Ranges->size(), 1);
+  LVLocations::const_iterator IterLocation = Ranges->begin();
----------------
probinson wrote:
> You can't continue if Ranges is null; should be able to continue if size() is wrong.
Changed to
```
  ASSERT_NE(Ranges, nullptr);
  ASSERT_EQ(Ranges->size(), 1u);
```
If `Ranges` is empty, 
```
  LVLocations::const_iterator IterLocation = Ranges->begin();
  LVLocation *Location = (*IterLocation);
```
It will crash trying to access `Location` (Invalid iterator).


================
Comment at: llvm/unittests/DebugInfo/LogicalView/CodeViewReaderTest.cpp:124-125
+  const LVLines *Lines = Foo->getLines();
+  EXPECT_NE(Lines, nullptr);
+  ASSERT_EQ(Lines->size(), 0x10);
+}
----------------
probinson wrote:
> 
Changed to
```
  ASSERT_NE(Lines, nullptr);
  EXPECT_EQ(Lines->size(), 0x10u);

```


================
Comment at: llvm/unittests/DebugInfo/LogicalView/CodeViewReaderTest.cpp:150-151
+  const LVLocations *Ranges = Function->getRanges();
+  EXPECT_NE(Ranges, nullptr);
+  ASSERT_EQ(Ranges->size(), 1);
+  LVLocations::const_iterator IterLocation = Ranges->begin();
----------------
probinson wrote:
> 
Changed to
```
  ASSERT_NE(Ranges, nullptr);
  ASSERT_EQ(Ranges->size(), 1u);
```
If `Ranges` is empty, 
```
  LVLocations::const_iterator IterLocation = Ranges->begin();
  LVLocation *Location = (*IterLocation);
```
It will crash trying to access `Location` (Invalid iterator).


================
Comment at: llvm/unittests/DebugInfo/LogicalView/CodeViewReaderTest.cpp:190-191
+  const LVLines *Lines = Foo->getLines();
+  EXPECT_NE(Lines, nullptr);
+  ASSERT_EQ(Lines->size(), 0x0e);
+}
----------------
probinson wrote:
> 
Changed to:
```
  ASSERT_NE(Lines, nullptr);
  EXPECT_EQ(Lines->size(), 0x0eu);
```


================
Comment at: llvm/unittests/DebugInfo/LogicalView/CodeViewReaderTest.cpp:216-217
+  const LVLocations *Ranges = Function->getRanges();
+  EXPECT_NE(Ranges, nullptr);
+  ASSERT_EQ(Ranges->size(), 1);
+  LVLocations::const_iterator IterLocation = Ranges->begin();
----------------
probinson wrote:
> 
Changed to
```
  ASSERT_NE(Ranges, nullptr);
  ASSERT_EQ(Ranges->size(), 1u);
```
If `Ranges` is empty, 
```
  LVLocations::const_iterator IterLocation = Ranges->begin();
  LVLocation *Location = (*IterLocation);
```
It will crash trying to access `Location` (Invalid iterator).


================
Comment at: llvm/unittests/DebugInfo/LogicalView/CodeViewReaderTest.cpp:256-257
+  const LVLines *Lines = Foo->getLines();
+  EXPECT_NE(Lines, nullptr);
+  ASSERT_EQ(Lines->size(), 0x0e);
+}
----------------
probinson wrote:
> 
Changed to:
```
  ASSERT_NE(Lines, nullptr);
  EXPECT_EQ(Lines->size(), 0x0eu);
```


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

https://reviews.llvm.org/D125784