awarzynski added a comment.
Hi All,
I've just noticed this patch and realised that it takes `DiagnosticBuilder` in
the direction tangential to what we proposed in [1]. I've just restarted our
efforts with regard to refactoring these APIs and feel that it would be good to
coordinate any future efforts in this area. I also want to make sure that we
have a solution that works for everyone.
Basically, this patch makes `DiagnosticBuilder` more integrated with
libclangBasic by assuming that every diagnostic contains `SourceLocation`s and
`FixItHint`s (see the definition of `DiagnosticStorage`). However, that's not
the case for driver diagnostics (i.e. diagnostics issued in libclangDriver). In
[1] we proposed to create thin abstraction layers above all diagnostic classes
that do not depend on `SourceLocation` (or anything else from libclangBasic).
This patch makes `DiagnosticBuilder` _more_ dependant on `SourceLocation` and
in this respect refactors `DiagnosticBuilder` in a direction opposite to what
we proposed in [1].
Since this patch changes the `DiagnosticBuilder` API quite significantly, I am
not entirely sure how to proceed with refactoring it (we need to make it
independent of `SourceLocation` and any related classes). Originally,
`DiagnosticBuilder` was a standalone class, but now we will have to refactor
the following classes on top of `DiagnosticBuilder`:
- `DiagnosticStorage`
- `StreamingDiagnostic`
- `PartialDiagnostic`
One idea that immediately comes to mind is to split `DiagnosticStorage` as
follows:
struct DiagnosticStorage {
// Storage that applies to all diagnostics
// Original content, but without DiagRanges and FixItHints
};
struct LangDiagnosticStorage {
// Storage that applies only to diagnostics that contain ranges and
FixitHints - in practice these are language diagnostics
SmallVector<CharSourceRange, 8> DiagRanges;
SmallVector<FixItHint, 6> FixItHints;
LangDiagnosticStorage() = default;
}
How does this look? Sadly it will clutter this API, but I can't see any way
around it. One other extreme alternative is to:
- revert this patch
- create a thin abstraction layer for `DiagnosticBuilder` independent of
SourceLocation (as per our RFC [1])
- re-apply this patch (with some necessary modifications)
I appreciate that it is important to keep this API as clean and as lean as
possible. However,
- it's required by libclangDriver
- we want to make libclangDriver independent of Clang (so that it can be used
by Flang without the dependency on Clang).
Further refactoring will be required to achieve this. We sent 2 RFCs [1] [2]
earlier this year to cfe-dev to discuss this, so I hope that this won't come as
a surprise.
Do you have any suggestions?
CC @tra @rjmccall @rsmith @yaxunl
Thank you,
Andrzej
[1] http://lists.llvm.org/pipermail/cfe-dev/2020-July/066393.html
[2] http://lists.llvm.org/pipermail/llvm-dev/2020-June/141994.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
