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
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
