labath added a comment.
I'm impressed by the test. I've tried to think of an alternative strategy for
testing this (perhaps by adding a suitable mode to lldb-test), but I couldn't
come up with one that a reasonable one, so this might be the best we can do
right now. Thank you for taking the trouble to write that.
================
Comment at: lldb/unittests/Symbol/TestLineEntry.cpp:60-87
+#define EXPECTED_NO_ERROR(x)
\
+ if (std::error_code ASSERT_NO_ERROR_ec = x) {
\
+ llvm::SmallString<128> MessageStorage;
\
+ llvm::raw_svector_ostream Message(MessageStorage);
\
+ Message << #x ": did not return errc::success.\n"
\
+ << "error number: " << ASSERT_NO_ERROR_ec.value() << "\n"
\
+ << "error message: " << ASSERT_NO_ERROR_ec.message() << "\n";
\
----------------
aadsm wrote:
> jingham wrote:
> > These defines don't seem specific to this test, is there a more general
> > place you could put them?
> I could put it in `TestUtilities.h`?
I don't think most of these should exist actually... Instead of
`EXPECTED_NO_ERROR`, you can use write `if(std::error_code ec = ...) return
llvm::errorCodeToError(ec);`
For the rest, I'd suggest just inlining the macros and using
`llvm::createStringError` instead of `llvm::make_error<llvm::StringError>`, as
it's somewhat shorter (if you want, you can make a utility wrapper function
around that to avoid the inconvertibleErrorCode argument).
================
Comment at: lldb/unittests/Symbol/TestLineEntry.cpp:89-94
+#define DUMP_RANGE(range)
\
+ {
\
+ StreamString s;
\
+ range.DumpDebug(&s);
\
+ std::cout << s.GetData() << std::endl;
\
+ }
----------------
This is unused (and it wouldn't be right anyway).
================
Comment at: lldb/unittests/Symbol/TestLineEntry.cpp:96-122
+llvm::Expected<ModuleSP> LineEntryTest::GetModule() {
+ if (m_module_sp)
+ return m_module_sp;
+
+ std::string yaml = GetInputFilePath("inlined-functions.yaml");
+ llvm::SmallString<128> obj;
+
----------------
aadsm wrote:
> jingham wrote:
> > It looks like this bit of business has been copied around in a bunch of
> > other tests in the unittest framework. Could we put this in a common place
> > (like make a LLDBUnitTest : testing::Test class that does this?
> How about adding a new function to TestUtilities.cpp named `ReadYAMLObject`?
A utility function sounds nice. (a test class would be fine too, but I'd name
it a bit less generic, as not all of our unit tests are in business of running
yaml2obj and creating modules).
The part I'm not so sure about is the location. Originally the idea was that we
would have a special subfolder for test helpers related to each module under
test, but then at some point that got changed into `TestingSupport` which
sounds more generic (this evolution here is visible in the fact that the cmake
target name in that folder is called `lldbUtilityHelpers`). If we put this
function there then we'd have to pull in the Core module (and everything that
goes with it). Though that isn't that bad on it's own, it is a bit unfortunate,
as right now the `Utility` unit test executable is our best defense against
unexpected dependencies creeping into the main module. After this, that
executable would link in the whole world again, and we'd lose this defense.
Another possibility might be to just put the yaml2obj (which is the main source
of mess here) part in that file, and leave the construction of the Module
object to the users.
================
Comment at: lldb/unittests/Symbol/TestLineEntry.cpp:150-152
+ auto line_entry = GetLineEntryForLine(17);
+ if (!line_entry)
+ ASSERT_TRUE(false) << line_entry.takeError();
----------------
replace with `ASSERT_THAT_EXPECTED(..., llvm::Succeeded());` (and in subsequent
tests)
================
Comment at: lldb/unittests/Symbol/TestLineEntry.cpp:208
+# inlined-functions.cpp is src.cpp for space considerations.
+0x20: outl %eax, %dx src.cpp:16
+0x21: movl $0xbeefdead, %esi src.cpp:16
----------------
The instructions aren't real (MachO obj2yaml does not preserve them), so it may
be best to just leave them out to avoid confusion.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D61292/new/
https://reviews.llvm.org/D61292
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
