[PATCH] D90006: Exposes interface to free up caching data structure in DWARFDebugLine and DWARFUnit for memory management

[Florin Papa via Phabricator via llvm-commits]

florinpapa added inline comments.


================
Comment at: llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp:402
+      LineData, SecondOffset, *Context, nullptr, RecordRecoverable);
+  ASSERT_TRUE(ExpectedLineTable4.operator bool());
+  EXPECT_FALSE(Recoverable);
----------------
dblaikie wrote:
> Could this be written as:
> ```
> ASSERT_TRUE(ExpectedLineTable4);
> ```
> ? (similarly for other bool tests - in general, explicitly calling operator overloads is a bit "weird" and best avoided if possible - if there's an issue with gtest only testing implicit conversion, then maybe it'd be suitable to explicitly cast to bool: `ASSERT_true((bool)ExpectedLineTable4)`)
> if there's an issue with gtest only testing implicit conversion, then maybe it'd be suitable to explicitly cast to bool: ASSERT_true((bool)ExpectedLineTable4))

How do I test that? I have been using `ninja check-llvm-unit` for testing so far, is that enough?


================
Comment at: llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp:402
+      LineData, SecondOffset, *Context, nullptr, RecordRecoverable);
+  ASSERT_TRUE(ExpectedLineTable4.operator bool());
+  EXPECT_FALSE(Recoverable);
----------------
florinpapa wrote:
> dblaikie wrote:
> > Could this be written as:
> > ```
> > ASSERT_TRUE(ExpectedLineTable4);
> > ```
> > ? (similarly for other bool tests - in general, explicitly calling operator overloads is a bit "weird" and best avoided if possible - if there's an issue with gtest only testing implicit conversion, then maybe it'd be suitable to explicitly cast to bool: `ASSERT_true((bool)ExpectedLineTable4)`)
> > if there's an issue with gtest only testing implicit conversion, then maybe it'd be suitable to explicitly cast to bool: ASSERT_true((bool)ExpectedLineTable4))
> 
> How do I test that? I have been using `ninja check-llvm-unit` for testing so far, is that enough?
I had to go with the latter, as the build failed without the explicit cast.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90006