labath accepted this revision.
labath added a comment.
The test changes look fine to me. Thank you for doing that.
================
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:
> 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?
Yeah, that's fine. I can think of a couple of ways of simplifying that further,
but that would require making some tweaks to the FileRemover class (the class
only has a handful of uses in llvm, so it's not surprising it has some rough
edges). However, this is already a pretty big improvement over the current
state, so thank you for doing that.
================
Comment at: lldb/unittests/TestingSupport/TestUtilities.cpp:12
#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/FormatVariadic.h"
#include "llvm/Support/Path.h"
----------------
What is this header used for? If need something to include to use
`createStringError`, you should include `llvm/Support/Error.h`, as that's what
defines it.
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
