[PATCH] D87774: [flang] Introduce DiagnosticConsumer classes in libflangFrontend

Thu Sep 24 11:23:21 PDT 2020

sameeranjoshi added inline comments.

================
Comment at: flang/lib/Frontend/TextDiagnostic.cpp:26
+static const enum llvm::raw_ostream::Colors savedColor =
+    llvm::raw_ostream::SAVEDCOLOR;
+
----------------
awarzynski wrote:
> sameeranjoshi wrote:
> > Unless Flang is not changing the color scheme w.r.t clang, can't the code be shared between both projects?
> > 
> That would be preferable, I agree. However, keep in mind that in Clang these enums are defined in  llvm-project/clang/lib/Frontend/TextDiagnostic.cpp , i.e. in `libclangFrontend`.  As suggested in [1], we shouldn't be re-using anything from `libclangFrontend`. 
> 
> We could move these enums out of `libclangFrontend` to some other library. That's not difficult and not that much work, but IMO that's a separate change that should happen on top of this patch (i.e. once this patch is approved). Also - where do we move them to? Currently there is no good candidate, but once the refactoring proposed in [1] is complete, there should be a dedicated library with infrastructure for the drivers to share.
> 
> I suggest that for now we just add a comment that recommends sharing these enums with Clang in the future. This could happen once we start refactoring Clang. In the meantime I'd like to keep the changes in Clang to the required minimum. 
> 
> How does this sound? And does it make sense? 
> 
> [1] http://lists.llvm.org/pipermail/cfe-dev/2020-July/066393.html
Agreed.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87774/new/

https://reviews.llvm.org/D87774