awarzynski added a comment.
Hi @rjmccall , thank you for your quick reply!
> It sounds like what you want is a diagnostic library that's almost completely
> abstracted over the kinds of entities that can be stored in a diagnostic,
> including the definition of a source location.
No :) We suggested (and that's based on the feedback on cfe-dev) a more basic
approach. We proposed an abstraction layer that does not require any location
information, because that's irrelevant in the driver (more on this below). We
did, intially, suggest re-using and re-factoring `SourceLocation` (and, more
specifically, `SourceManager`). That direction was explicitely discouraged on
cfe-dev [3].
> Source locations, supplemental source ranges, and fix-its are all still
> meaningful concepts in the driver
When we studied the source code of libclangDriver that was not the case. The
diagnostics in the driver [1] are only used to a very limited extent (compared
to the frontend) and I've only seen them issued via [2]:
inline DiagnosticBuilder DiagnosticsEngine::Report(unsigned DiagID) {
return Report(SourceLocation(), DiagID);
}
i.e. `SourceLocation` is not really used at all. Are we missing something here?
Is there any place in the libclangDriver (or any tool that's implemented in
terms of libclangdDriver) that actually requires `SourceLocation`?
We would like to keep the scope of refactoring libclangDriver (and its
dependencies) to the required minimum. The solution that you suggested would
mean refactoring an API that's not required by libclangDriver (please correct
me if I'm missing something here). I'm concerned that any attempts to modify
`SourceLocation` will be challenged on cfe-dev. It was already suggested that
we shouldn't need it for what we want to achieve. And indeed, in our simplified
prototype downstream we were able to avoid using it without affecting the
driver diagnostics.
Another difficulty with your suggestion is testing. If we were to create some
abstraction layer above `SourceLocation` - how would we define or test it?
Again, AFAIK diagnostics in libclangDriver never contain any information with
respect to source location. So how do we decide what's actually required? This
would be easy if there was a reference example :)
All in all, I think that this patch will require us to broaden the refactoring
in a way that otherwise wouldn't be required. And according to the feedback so
far this will be detrimental to Clang. If we are still in disagreement then
perhaps we should move this discussion to cfe-dev? I want to make sure that we
are all aligned on the overall direction and that we don't diverge too much
from what was already discussed on cfe-dev.
Thanks in advance for your feedback,
-Andrzej
[1]
https://github.com/llvm/llvm-project/blob/3c3071d5e7dd259022d4f5e879fdf9824b9e490c/clang/lib/Driver/OptionUtils.cpp#L26
https://github.com/llvm/llvm-project/blob/3c3071d5e7dd259022d4f5e879fdf9824b9e490c/clang/lib/Driver/Driver.cpp#L270
https://github.com/llvm/llvm-project/blob/3c3071d5e7dd259022d4f5e879fdf9824b9e490c/clang/lib/Driver/Driver.cpp#L275
https://github.com/llvm/llvm-project/blob/3c3071d5e7dd259022d4f5e879fdf9824b9e490c/clang/lib/Driver/Driver.cpp#L1145
[2]
https://github.com/llvm/llvm-project/blob/3c3071d5e7dd259022d4f5e879fdf9824b9e490c/clang/include/clang/Basic/Diagnostic.h#L1482-L1484
[3] http://lists.llvm.org/pipermail/cfe-dev/2020-June/065766.html
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D84362/new/
https://reviews.llvm.org/D84362
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
