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

[sameeran joshi via Phabricator via cfe-commits]

sameeranjoshi added a comment.

Thanks for working on it.
Few comments inline:

1. For an out-of-tree build, I see `check-flang` target failing with

  /unittests/Frontend/CompilerInstanceTest.cpp:17:10: fatal error: filesystem: No such file or directory
   #include <filesystem>
            ^~~~~~~~~~~~

I used gcc/g++ 7.5 version.
I haven't checked in-tree still, and others/bots might have checked it.

2. Either the documentation comments are wrong or code.

`README` mentions `DBUILD_FLANG_NEW_DRIVER` where as cmake ignores the flag for me.
Whereas, `CMakeLists.txt` mentions `FLANG_BUILD_NEW_DRIVER`.



================
Comment at: flang/include/flang/Frontend/TextDiagnosticBuffer.h:36
+  /// level and an index into the corresponding DiagList above.
+  std::vector<std::pair<clang::DiagnosticsEngine::Level, size_t>> all_;
+
----------------
How about simplifying this with `using` keyword?


================
Comment at: flang/include/flang/Frontend/TextDiagnosticPrinter.h:37
+  /// Handle to the currently active text diagnostic emitter.
+  std::unique_ptr<TextDiagnostic> textDiag_;
+
----------------
Where is this used? I don't see any reference.


================
Comment at: flang/lib/Frontend/TextDiagnostic.cpp:12
+#include "llvm/Support/raw_ostream.h"
+#include <algorithm>
+
----------------
Is this header used ?


================
Comment at: flang/lib/Frontend/TextDiagnostic.cpp:26
+static const enum llvm::raw_ostream::Colors savedColor =
+    llvm::raw_ostream::SAVEDCOLOR;
+
----------------
Unless Flang is not changing the color scheme w.r.t clang, can't the code be shared between both projects?



================
Comment at: flang/lib/Frontend/TextDiagnosticPrinter.cpp:24
+
+TextDiagnosticPrinter::TextDiagnosticPrinter(
+    raw_ostream &os, clang::DiagnosticOptions *diags)
----------------
A silly question from what I see usually in Flang coding style.
Why isn't it defined in header file?


================
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);
----------------
kiranchandramohan wrote:
> 100? Will this contain path names by any chance?
Can we use at places where LLVM data structures are used explicit `llvm::` so an unknown user can easily identify where they come from?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87774