[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

[Andrzej Warzynski via Phabricator via cfe-commits]

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