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

[Andrzej Warzynski via Phabricator via cfe-commits]

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