zturner added inline comments.
================
Comment at: source/Core/ModuleList.cpp:94
- llvm::SmallString<128> path;
- clang::driver::Driver::getDefaultModuleCachePath(path);
- SetClangModulesCachePath(path);
+ assert(!g_default_clang_modules_cache_path.empty());
+ SetClangModulesCachePath(g_default_clang_modules_cache_path);
----------------
zturner wrote:
> zturner wrote:
> > aprantl wrote:
> > > zturner wrote:
> > > > zturner wrote:
> > > > > zturner wrote:
> > > > > > aprantl wrote:
> > > > > > > zturner wrote:
> > > > > > > > aprantl wrote:
> > > > > > > > > zturner wrote:
> > > > > > > > > > I don't think this should be an assert. After all, if the
> > > > > > > > > > whole point is to make LLDB usable in situations where
> > > > > > > > > > clang is not present, then someone using it in such an
> > > > > > > > > > environment would probably never call the static function
> > > > > > > > > > to begin with. So I think we should just remove the assert
> > > > > > > > > > and set it to whatever the value happens to be (including
> > > > > > > > > > empty)
> > > > > > > > > The assertion enforces that
> > > > > > > > > ModuleListProperties::Initialize() has been called. If we
> > > > > > > > > want to make it more convenient, we can add a default
> > > > > > > > > argument `= "dummy"` for clients that don't link against
> > > > > > > > > clang.
> > > > > > > > I was actually thinking that instead of calling it `Initialize`
> > > > > > > > (which sounds generic and like it's required) we would just
> > > > > > > > call it `SetDefaultClangModulesCachePath` and have the user
> > > > > > > > directly call that. With a name like `Initialize`, it makes
> > > > > > > > the user think that it's required, but in fact the only thing
> > > > > > > > it's initializing is something that is optional, so it
> > > > > > > > shouldn't be required.
> > > > > > > >
> > > > > > > > It's possible I'm misunderstanding something though.
> > > > > > > My point was that this *is* required (for all clients of lldb
> > > > > > > that also link against clang). When LLDB initializes clang it
> > > > > > > must set a module cache path because clang doesn't implement a
> > > > > > > fallback.
> > > > > > If there's a client of LLDB using the public API and/or clang then
> > > > > > that client would also be using `SystemInitializerFull` (or at the
> > > > > > very least, would be responsible for initializing the set of things
> > > > > > they need, one of which would be this path).
> > > > > >
> > > > > > My point is that `Core` should ultimately have no knowledge that
> > > > > > something called clang even exists, and it definitely shouldn't be
> > > > > > limiting the use of itself based on the needs of a specific client
> > > > > > since it something that is useful to all clients. If a particular
> > > > > > client requires clang, that client should initialize clang.
> > > > > >
> > > > > > With an assert, this is requiring a non clang-based client to run
> > > > > > some initialization code that is only required for a clang-based
> > > > > > client, which doesn't seem like a reasonable restriction (imagine
> > > > > > if every downstream developer using every possible set of random
> > > > > > 3rd party libraries started asserting in low-level debugger code
> > > > > > that their optional component had been initialized).
> > > > > In short, `Core` is too low level to be making any assumptions
> > > > > whatsoever about the needs of a particular client. It may be
> > > > > required for all clients of lldb that use clang, but `Core` is not
> > > > > the right place to be making decisions based on whether a client of
> > > > > lldb uses clang (or any other optional external library / component).
> > > > To put this in perspective, imagine if LLVM's optimization pass library
> > > > had something like `assert(driverIsClang());`
> > > The assertion is not supposed to check that Clang has been initialized.
> > > It is supposed to check that ModuleListProperties::Initialize() has been
> > > called. The fact that in order to call this function a client may want to
> > > get a string from the Clang Driver is an (ugly) implementation detail.
> > > And clients that don't use clang (such as the confusingly named unit
> > > tests) can pass in any nonempty string (which as I offered earlier could
> > > be made into a default argument).
> > But why must it even be a nonempty string? And for that matter, if they're
> > not going to use clang anyway, why even require the function to be called
> > in the first place? If it were an initialization function that did
> > multiple things, it might be a stronger argument. But as it stands, its
> > only purpose is, in fact, to set a value for this path, which people who
> > aren't using clang shouldn't be required to do.
> >
> > This is making a decision in a low level library for the purpose of 1
> > specific client, which doesn't seem right. I'm not entirely opposed to an
> > assert, but it should only happen in clients that are using clang,
> > otherwise this is effectively 'assert that the user has executed a no-op',
> > which doesn't make sense.
> > I'm not entirely opposed to an assert, but it should only happen in clients
> > that are using clang
> (and hence not in `Core` but in something higher level like ClangASTContext,
> or the place where you actually make use of this path).
For example, the only place this appears to be used is in
`ClangModulesDeclVendor.cpp` line 595. It looks like this:
```
props.GetClangModulesCachePath().GetPath(path);
```
How about adding `assert(!path.empty());` after that?
https://reviews.llvm.org/D47235
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
