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