[Lldb-commits] [PATCH] D66357: Fix GetDIEForDeclContext so it only returns entries matching the provided context

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D66357#1635851 <https://reviews.llvm.org/D66357#1635851>, @clayborg wrote:

> Pavel, any comments on the testing code?


I am worried that this approach of using pointers into thin air instead of real objects (which admittedly, would be hard to create/mock) is going to make this test fragile in face of future modifications of this code (if, e.g., something suddenly decides it needs to dereference these pointers).

However, given that this patch is (if I understand correctly) fixing a performance bug, and not an actual correctness bug, and we don't really have a framework for testing performance, I would be fine with accepting the patch without a test, and so I suppose I would be also fine with just deleting this test if/when it starts to become a maintenance burden...



================
Comment at: lldb/unittests/SymbolFile/DWARF/DWARFASTParserClangTests.cpp:18-19
+namespace {
+class DWARFASTParserClangTests : public testing::Test {};
+
+class DWARFASTParserClangStub : public DWARFASTParserClang {
----------------
If you're not going to be putting any code here, you can just drop the fixture and use `TEST` instead of `TEST_F` below.


================
Comment at: lldb/unittests/SymbolFile/DWARF/DWARFASTParserClangTests.cpp:32-36
+  auto unit = (DWARFUnit *)nullptr;
+  auto die1 = DWARFDIE(unit, (DWARFDebugInfoEntry *)1LL);
+  auto die2 = DWARFDIE(unit, (DWARFDebugInfoEntry *)2LL);
+  auto die3 = DWARFDIE(unit, (DWARFDebugInfoEntry *)3LL);
+  auto die4 = DWARFDIE(unit, (DWARFDebugInfoEntry *)4LL);
----------------
This seems to be written in the "always use auto" style. That is definitely not consistent with the llvm coding style <http://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable>, as there's a shorter, more traditional way to write this, and the style guide explicitly rejects "always use auto".


================
Comment at: lldb/unittests/SymbolFile/DWARF/DWARFASTParserClangTests.cpp:44
+      CompilerDeclContext(nullptr, (clang::DeclContext *)2LL));
+  EXPECT_EQ(2u, die_list.size());
+  EXPECT_EQ(die2, die_list[0]);
----------------
s/EXPECT/ASSERT


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66357