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