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