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

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added a comment.

I had a quick look through this patch. Have a few comments (mostly nits and questions) inline.



================
Comment at: flang/include/flang/Frontend/CompilerInvocation.h:18
+
+/// Fill out Opts based on the options given in Args.
+///
----------------
Nit: Opts -> opts?


================
Comment at: flang/include/flang/Frontend/CompilerInvocation.h:20
+///
+/// When errors are encountered, return false and, if Diags is non-null,
+/// report the error(s).
----------------
Nit: Diags -> diags? Or is that convention?


================
Comment at: flang/include/flang/Frontend/TextDiagnostic.h:9
+//
+// An utility class that provides support for textual pretty-printing of
+// diagnostics. Based on clang::TextDiagnostic (this is a trimmed version).
----------------
An -> A


================
Comment at: flang/include/flang/Frontend/TextDiagnostic.h:41
+
+  /// Print the diagonstic level to a llvm::raw_ostream.
+  ///
----------------
Nit: diagonstic -> diagnostic


================
Comment at: flang/include/flang/Frontend/TextDiagnostic.h:44
+  /// This is a static helper that handles colorizing the level and formatting
+  /// it into an arbitrary output stream.
+  static void printDiagnosticLevel(llvm::raw_ostream &os,
----------------
Nit: missed doxygen comments? Or is it because it is a helper function?


================
Comment at: flang/include/flang/Frontend/TextDiagnostic.h:56
+  ///
+  /// \param OS Where the message is printed
+  /// \param isSupplemental true if this is a continuation note diagnostic
----------------
Nit: OS->os?


================
Comment at: flang/include/flang/Frontend/TextDiagnosticPrinter.h:10
+// This is a concrete diagnostic client, which prints the diagnostics to
+// standard error.
+//
----------------
Where it prints depends on what stream is passed to the constructor of this class right? So is this standard error usage correct here?


================
Comment at: flang/include/flang/Frontend/TextDiagnosticPrinter.h:14
+
+#ifndef LLVM_FLANG_FRONTEND_TEXTDIAGNosTICPRINTER_H
+#define LLVM_FLANG_FRONTEND_TEXTDIAGNosTICPRINTER_H
----------------
Nit: Nos->NOS?


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


================
Comment at: flang/lib/Frontend/TextDiagnostic.cpp:22-23
+    llvm::raw_ostream::MAGENTA;
+static const enum llvm::raw_ostream::Colors errorColor = llvm::raw_ostream::RED;
+static const enum llvm::raw_ostream::Colors fatalColor = llvm::raw_ostream::RED;
+// Used for changing only the bold attribute.
----------------
Is the plan to make  all these colors configurable?


================
Comment at: flang/lib/Frontend/TextDiagnostic.cpp:69-74
+  case clang::DiagnosticsEngine::Error:
+    os << "error";
+    break;
+  case clang::DiagnosticsEngine::Fatal:
+    os << "fatal error";
+    break;
----------------
Tempted to suggest saving a break here by reordering. :) 


================
Comment at: flang/lib/Frontend/TextDiagnosticBuffer.cpp:28
+
+  clang::SmallString<100> buf;
+  info.FormatDiagnostic(buf);
----------------
Is that a different smallstring from the one in LLVM?


================
Comment at: flang/lib/Frontend/TextDiagnosticPrinter.cpp:37
+  // this later as we print out the diagnostic to the terminal.
+  SmallString<100> outStr;
+  info.FormatDiagnostic(outStr);
----------------
100? Will this contain path names by any chance?


================
Comment at: flang/lib/Frontend/TextDiagnosticPrinter.cpp:42
+
+  if (!prefix_.empty())
+    os_ << prefix_ << ": ";
----------------
Is there an assumption that the prefix will not be empty?


================
Comment at: flang/test/Flang-Driver/driver-help.f90:10
 ! CHECK-NEXT:OPTIONS:
+! CHECK-NEXT: -fcolor-diagnostics    Enable colors in diagnostics
+! CHECK-NEXT: -fno-color-diagnostics Disable colors in diagnostics
----------------
Should there be tests for gcc style as well?


================
Comment at: flang/tools/flang-driver/driver.cpp:45
+
+  // Inore missingArgCount and the return value of ParseDiagnosticArgs.
+  // Any errors that would be diagnosed here will also be diagnosed later,
----------------
Nit: Inore->Ignore


================
Comment at: flang/tools/flang-driver/driver.cpp:104
+  diagClient->set_prefix(
+      std::string(llvm::sys::path::stem(GetExecutablePath(argv[0]))));
+
----------------
Not clear why the stem needs to be taken here. Is this for windows compatibility?


================
Comment at: flang/tools/flang-driver/fc1_main.cpp:39
+  // them using a well formed diagnostic object.
+  Fortran::TextDiagnosticBuffer *diagsBuffer =
+      new Fortran::TextDiagnosticBuffer;
----------------
Anyreason why this was moved up here (compared to the previous version which was close to its use)?


================
Comment at: flang/tools/flang-driver/fc1_main.cpp:40
+  Fortran::TextDiagnosticBuffer *diagsBuffer =
+      new Fortran::TextDiagnosticBuffer;
+
----------------
#JustAsking: Should this be deleted somewhere?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87774