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

[Caroline via Phabricator via cfe-commits]

CarolineConcatto added a comment.

Thank you @awarzynski  for this patch.
Very good that you added color feature for Fortran.
It is just a shame that we do not test. Is there a way to add a regression test for it?
I don't see any inconsistency with what we have on clang side, so it is good.
My comments are in the code, but two things that need an overall look:
1- style is not yet according to Flang, mainly functions and variables in the classes
2 -I believe that the name space is Fortran::frontend to be consistent with the other files at Frontend.



================
Comment at: clang/include/clang/Driver/Options.td:874
+defm color_diagnostics : OptInFFlag<"color-diagnostics", "Enable", "Disable", " colors in diagnostics",
+  [CoreOption, FlangOption]>;
 def fdiagnostics_color : Flag<["-"], "fdiagnostics-color">, Group<f_Group>,
----------------
Is it possible to also add when using FC1Option?


================
Comment at: clang/tools/driver/driver.cpp:281
       argv.slice(1), MissingArgIndex, MissingArgCount);
+
   // We ignore MissingArgCount and the return value of ParseDiagnosticArgs.
----------------
undo


================
Comment at: flang/include/flang/Frontend/TextDiagnostic.h:18
+
+#include "clang/Frontend/DiagnosticRenderer.h"
+#include "llvm/ADT/IntrusiveRefCntPtr.h"
----------------
Do we need clang/Frontend here?


================
Comment at: flang/include/flang/Frontend/TextDiagnostic.h:60
+  /// \param showColors Enable colorizing of the message.
+  static void printDiagnosticMessage(llvm::raw_ostream &os, bool isSupplemental,
+      llvm::StringRef message, bool showColors);
----------------
According to Flang style It should be PrintDiagnosticMessage, no?


================
Comment at: flang/include/flang/Frontend/TextDiagnosticBuffer.h:23
+
+namespace Fortran {
+
----------------
It is inside Frontend, why you are not using Fortran::frontend?


================
Comment at: flang/include/flang/Frontend/TextDiagnosticBuffer.h:42
+
+  /// FlushDiagnostics - Flush the buffered diagnostics to an given
+  /// diagnostic engine.
----------------
a given


================
Comment at: flang/include/flang/Frontend/TextDiagnosticPrinter.h:48
+  /// start of any diagnostics. If empty, no prefix string is used.
+  void set_prefix(std::string value) { prefix_ = std::move(value); }
+
----------------
style


================
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:75
+bool Fortran::frontend::ParseDiagnosticArgs(clang::DiagnosticOptions &opts,
+    llvm::opt::ArgList &args, clang::DiagnosticsEngine *diags,
+    bool defaultDiagColor) {
----------------
kiranchandramohan wrote:
> Is diags not needed now?
Why do you have clang::DiagnosticsEngine *diags


================
Comment at: flang/lib/Frontend/TextDiagnostic.cpp:32
+
+/*static*/ void TextDiagnostic::printDiagnosticLevel(llvm::raw_ostream &os,
+    clang::DiagnosticsEngine::Level level, bool showColors) {
----------------
Why do static is commented?


================
Comment at: flang/lib/Frontend/TextDiagnosticPrinter.cpp:52
+  Fortran::TextDiagnostic::printDiagnosticMessage(os_,
+      /*IsSupplemental=*/level == clang::DiagnosticsEngine::Note,
+      DiagMessageStream.str(), diagOpts_->ShowColors);
----------------
Why when it is clang::DiagnosticsEngine::Note it is Supplemental?


================
Comment at: flang/tools/flang-driver/driver.cpp:48
+  // when the DiagnosticsEngine actually exists.
+  unsigned missingArgIndex, missingArgCount;
+  llvm::opt::InputArgList args = clang::driver::getDriverOptTable().ParseArgs(
----------------
This is an odd part of the code, because  missingArgIndex, missingArgCount are not set or even used in this part of the code.


================
Comment at: flang/tools/flang-driver/driver.cpp:52
+      /*FlagsToInclude=*/clang::driver::options::FlangOption);
+  (void)Fortran::frontend::ParseDiagnosticArgs(*diagOpts, args);
+
----------------
can you do only diagOpts.showcolor= parseShowColorsArgs(args, defaultDiagColor)?
this function is not doing much now and it only adds another layer to understand the code.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87774