aadsm marked an inline comment as done.
aadsm added a comment.
@labath thank you for your kind words!
================
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;
+
----------------
labath wrote:
> 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.
Yeah, the way I did it locally was to create a function that only handles the
yaml2obj part. Some tests use the file for other things other than creating a
ModuleSpec.
I also put the responsibility of creating and cleaning up the object file in
the caller. I was not sure how to handle the clean up of the temporary file for
all different cases. Here's how it looks like from the caller side.
```
llvm::SmallString<128> obj;
if (auto ec = llvm::sys::fs::createTemporaryFile("source-%%%%%%", "obj", obj))
return llvm::errorCodeToError(ec);
llvm::FileRemover obj_remover(obj);
if (auto error = ReadYAMLObjectFile("inlined-functions.yaml", obj))
return llvm::Error(std::move(error));
```
What do you think?
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
