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
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to