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