[PATCH] D87774: [flang] Introduce DiagnosticConsumer classes in libflangFrontend
Andrzej Warzynski via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Sep 23 09:57:49 PDT 2020
awarzynski added a comment.
Thank you for reviewing! I think that I've addressed all your comments.. Please see the updated patch.
================
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>,
----------------
CarolineConcatto wrote:
> Is it possible to also add when using FC1Option?
I skipped it intentionally. In fc1_main.cpp we use `TextDiagnosticBuffer` instead of `TextDiagnosticPrinter`. The former will just print the diagnostics without pretty-formatting them, so this option wouldn't make much sense.
We could use `TextDiagnosticPrinter` in `flang-new -fc1` (frontend driver) as we do for `flang-new` (compiler driver), but at this stage I would prefer to keep the design consistent with `clang` (i.e. this is the main reason why we use `TextDiagnosticBuffer` instead of `TextDiagnosticPrinter` in the frontend driver).
Having said all that, your comment makes me realise that I should improve the documentation, thanks!
================
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).
----------------
kiranchandramohan wrote:
> Nit: Diags -> diags? Or is that convention?
Copy and paste error, thanks!
================
Comment at: flang/include/flang/Frontend/TextDiagnostic.h:18
+
+#include "clang/Frontend/DiagnosticRenderer.h"
+#include "llvm/ADT/IntrusiveRefCntPtr.h"
----------------
CarolineConcatto wrote:
> Do we need clang/Frontend here?
This should be replaced with `#include "clang/Basic/Diagnostic.h"`. Thanks for pointing out!
================
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);
----------------
CarolineConcatto wrote:
> According to Flang style It should be PrintDiagnosticMessage, no?
Correct, thanks for catching this!
================
Comment at: flang/include/flang/Frontend/TextDiagnosticBuffer.h:23
+
+namespace Fortran {
+
----------------
CarolineConcatto wrote:
> It is inside Frontend, why you are not using Fortran::frontend?
I've just copied the design from Clang - that's a rather disappointing explanation. But I agree that `Fortran::frontend` would be better. Updated.
================
Comment at: flang/include/flang/Frontend/TextDiagnosticPrinter.h:10
+// This is a concrete diagnostic client, which prints the diagnostics to
+// standard error.
+//
----------------
kiranchandramohan wrote:
> Where it prints depends on what stream is passed to the constructor of this class right? So is this standard error usage correct here?
Good catch. I've updated this and also the description of `TextDiagnosticBuffer`. Hopefully that will make the distinction clearer.
================
Comment at: flang/include/flang/Frontend/TextDiagnosticPrinter.h:37
+ /// Handle to the currently active text diagnostic emitter.
+ std::unique_ptr<TextDiagnostic> textDiag_;
+
----------------
sameeranjoshi wrote:
> Where is this used? I don't see any reference.
We won't be needing this :) Thanks for catching this!
================
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); }
+
----------------
CarolineConcatto wrote:
> style
I think that this is correct as is:
> Accessor member functions are named with the non-public data member's name, less the trailing underscore.
https://github.com/llvm/llvm-project/blob/master/flang/docs/C%2B%2Bstyle.md#naming
3rd point
================
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:75
+bool Fortran::frontend::ParseDiagnosticArgs(clang::DiagnosticOptions &opts,
+ llvm::opt::ArgList &args, clang::DiagnosticsEngine *diags,
+ bool defaultDiagColor) {
----------------
CarolineConcatto wrote:
> kiranchandramohan wrote:
> > Is diags not needed now?
> Why do you have clang::DiagnosticsEngine *diags
I missed that one. Thank you @CarolineConcatto and @kiranchandramohan !
================
Comment at: flang/lib/Frontend/TextDiagnostic.cpp:12
+#include "llvm/Support/raw_ostream.h"
+#include <algorithm>
+
----------------
sameeranjoshi wrote:
> Is this header used ?
Nope, thanks for catching this!
================
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.
----------------
kiranchandramohan wrote:
> Is the plan to make all these colors configurable?
No, why? This is a copy & paste from Clang: https://github.com/llvm/llvm-project/blob/master/clang/lib/Frontend/TextDiagnostic.cpp#L26-L42.
================
Comment at: flang/lib/Frontend/TextDiagnostic.cpp:26
+static const enum llvm::raw_ostream::Colors savedColor =
+ llvm::raw_ostream::SAVEDCOLOR;
+
----------------
sameeranjoshi wrote:
> Unless Flang is not changing the color scheme w.r.t clang, can't the code be shared between both projects?
>
That would be preferable, I agree. However, keep in mind that in Clang these enums are defined in llvm-project/clang/lib/Frontend/TextDiagnostic.cpp , i.e. in `libclangFrontend`. As suggested in [1], we shouldn't be re-using anything from `libclangFrontend`.
We could move these enums out of `libclangFrontend` to some other library. That's not difficult and not that much work, but IMO that's a separate change that should happen on top of this patch (i.e. once this patch is approved). Also - where do we move them to? Currently there is no good candidate, but once the refactoring proposed in [1] is complete, there should be a dedicated library with infrastructure for the drivers to share.
I suggest that for now we just add a comment that recommends sharing these enums with Clang in the future. This could happen once we start refactoring Clang. In the meantime I'd like to keep the changes in Clang to the required minimum.
How does this sound? And does it make sense?
[1] http://lists.llvm.org/pipermail/cfe-dev/2020-July/066393.html
================
Comment at: flang/lib/Frontend/TextDiagnostic.cpp:32
+
+/*static*/ void TextDiagnostic::printDiagnosticLevel(llvm::raw_ostream &os,
+ clang::DiagnosticsEngine::Level level, bool showColors) {
----------------
CarolineConcatto wrote:
> Why do static is commented?
https://en.cppreference.com/w/cpp/language/static
```
The static keyword is only used with the declaration of a static member, inside the class definition, but not with the definition of that static member
```
================
Comment at: flang/lib/Frontend/TextDiagnostic.cpp:69-74
+ case clang::DiagnosticsEngine::Error:
+ os << "error";
+ break;
+ case clang::DiagnosticsEngine::Fatal:
+ os << "fatal error";
+ break;
----------------
kiranchandramohan wrote:
> Tempted to suggest saving a break here by reordering. :)
Do suggest it and paste an example - my C++ foo is failing me here :)
================
Comment at: flang/lib/Frontend/TextDiagnosticBuffer.cpp:28
+
+ clang::SmallString<100> buf;
+ info.FormatDiagnostic(buf);
----------------
kiranchandramohan wrote:
> Is that a different smallstring from the one in LLVM?
It's the same `SmallString`. I'm using a forward declaration from `clang/Basic/LLVM.h`, which I do include, but I shouldn't. Fixed!
================
Comment at: flang/lib/Frontend/TextDiagnosticPrinter.cpp:24
+
+TextDiagnosticPrinter::TextDiagnosticPrinter(
+ raw_ostream &os, clang::DiagnosticOptions *diags)
----------------
sameeranjoshi wrote:
> A silly question from what I see usually in Flang coding style.
> Why isn't it defined in header file?
No such thing as silly questions! :)
AFAIK, there are no rules in the coding guidelines with regard to
* where things should be defined, and
* where things should be declared.
I use this rather generic rule of thumb: declare in headers, define in source files. In this particular case - I followed what was already in Clang. It made sense to me.
Do you think that it would better to define this in a 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);
----------------
sameeranjoshi wrote:
> 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?
If we do that, the code is likely to be bloated with `llvm::`, which IMO could be detrimental to readability in the long term.
In Clang you will find a header file with all the forward declarations to avoid this: https://github.com/llvm/llvm-project/blob/master/clang/include/clang/Basic/LLVM.h.
I definitely want to make reading this code easier to users, but I also think that instead of `llvm::` one could use code-navigation tools (e.g. clangd). And forward declarations are fairly standard AFAIK.
However, if you still think that `llvm::` would be better, I'm happy to update :)
================
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);
----------------
awarzynski wrote:
> sameeranjoshi wrote:
> > 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?
> If we do that, the code is likely to be bloated with `llvm::`, which IMO could be detrimental to readability in the long term.
>
> In Clang you will find a header file with all the forward declarations to avoid this: https://github.com/llvm/llvm-project/blob/master/clang/include/clang/Basic/LLVM.h.
>
> I definitely want to make reading this code easier to users, but I also think that instead of `llvm::` one could use code-navigation tools (e.g. clangd). And forward declarations are fairly standard AFAIK.
>
> However, if you still think that `llvm::` would be better, I'm happy to update :)
It shouldn't. Currently these diagnostics are only used to report errors from the command line, so no paths are involved.
In Clang, similar class is used both for the driver diagnostics (i..e command line errors - no source location) and frontend diagnostics (source code errors - with source locations). However, AFAIK, this string is not used for the source location (e.g. path).
If we do use this class for more Flang diagnostics, we shouldn't use `outStr` for paths. This is just the message.
================
Comment at: flang/lib/Frontend/TextDiagnosticPrinter.cpp:42
+
+ if (!prefix_.empty())
+ os_ << prefix_ << ": ";
----------------
kiranchandramohan wrote:
> Is there an assumption that the prefix will not be empty?
The prefix is optional. With prefix:
```
$ bin/flang-new
flang-new: error: no input files
```
Without prefix:
```
$ bin/flang-new
error: no input files
```
Both are valid.
================
Comment at: flang/lib/Frontend/TextDiagnosticPrinter.cpp:52
+ Fortran::TextDiagnostic::printDiagnosticMessage(os_,
+ /*IsSupplemental=*/level == clang::DiagnosticsEngine::Note,
+ DiagMessageStream.str(), diagOpts_->ShowColors);
----------------
CarolineConcatto wrote:
> Why when it is clang::DiagnosticsEngine::Note it is Supplemental?
I'm not aware of any documentation for this, so here's my understanding :)
A `clang::DiagnosticsEngine::Note` is an additional piece of information for the user. It's neither a warning nor an error. Hence it's something _supplemental_ (i.e. additional info that could otherwise be skipped) and should not be pretty-printed. And that's the purpose of this argument.
================
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
----------------
kiranchandramohan wrote:
> Should there be tests for gcc style as well?
That's a good point,thanks! However, I'd like to defer this decision till later.
Currently we have enough options to support what is implemented in this driver. At some point we will need to have a discussion with regard to what options should be supported and displayed. This is not obvious to me just now. For example, Clang supports GCC style flags for diagnostic colors, but doesn't display them when running `clang --help`.
And once we do start discussing this in the context of Flang, any changes will very likely affect Clang too. I would prefer to focus on the required minimum before offering a complete set of supported option variants :)
================
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(
----------------
CarolineConcatto wrote:
> This is an odd part of the code, because missingArgIndex, missingArgCount are not set or even used in this part of the code.
They are required when calling `ParseArgs` (that's also where they are set). As mentioned in the comment above, we ignore them as any errors that these variables would indicate, will be captured elsewhere anyway.
================
Comment at: flang/tools/flang-driver/driver.cpp:52
+ /*FlagsToInclude=*/clang::driver::options::FlangOption);
+ (void)Fortran::frontend::ParseDiagnosticArgs(*diagOpts, args);
+
----------------
CarolineConcatto wrote:
> 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.
>
That's a good point, but then I have to move the definition of `parseShowColorsArgs` into this file. This will bloat this file with stuff that should be abstracted away.
================
Comment at: flang/tools/flang-driver/driver.cpp:104
+ diagClient->set_prefix(
+ std::string(llvm::sys::path::stem(GetExecutablePath(argv[0]))));
+
----------------
kiranchandramohan wrote:
> Not clear why the stem needs to be taken here. Is this for windows compatibility?
That too. In general, to canonicalise the path.
For example, we want:
```
$ bin/flang-new
flang-new: error: no input files
```
instead of:
```
$ bin/flang-new
<some-very-long-path>/bin/flang-new: error: no input files
```
================
Comment at: flang/tools/flang-driver/fc1_main.cpp:39
+ // them using a well formed diagnostic object.
+ Fortran::TextDiagnosticBuffer *diagsBuffer =
+ new Fortran::TextDiagnosticBuffer;
----------------
kiranchandramohan wrote:
> Anyreason why this was moved up here (compared to the previous version which was close to its use)?
Sorry, this is unrelated.
I wanted to better highlight that we are using `TextDiagnosticBuffer` (instead of e.g. `TextDiagnosticPrinter`) and to separate the creation of CompilerInvocation from this.
================
Comment at: flang/tools/flang-driver/fc1_main.cpp:40
+ Fortran::TextDiagnosticBuffer *diagsBuffer =
+ new Fortran::TextDiagnosticBuffer;
+
----------------
kiranchandramohan wrote:
> #JustAsking: Should this be deleted somewhere?
`DiagnosticsConsumer` (in this case, `diagsBuffer`), is required to construct `diags`, i.e. an instance of `DiagnosticsEngine`.
* the constructor for `DiagnosticsEngine`: https://github.com/llvm/llvm-project/blob/master/clang/lib/Basic/Diagnostic.cpp#L80-L89.
```
DiagnosticsEngine::DiagnosticsEngine(
IntrusiveRefCntPtr<DiagnosticIDs> diags,
IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts, DiagnosticConsumer *client,
bool ShouldOwnClient)
: Diags(std::move(diags)), DiagOpts(std::move(DiagOpts)) {
setClient(client, ShouldOwnClient);
ArgToStringFn = DummyArgToStringFn;
Reset();
}
```
* the prototype: https://github.com/llvm/llvm-project/blob/master/clang/include/clang/Basic/Diagnostic.h#L494-L497.
```
explicit DiagnosticsEngine(IntrusiveRefCntPtr<DiagnosticIDs> Diags,
IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts,
DiagnosticConsumer *client = nullptr,
bool ShouldOwnClient = true);
```
Note that `ShouldOwnClient` defaults to true. So this should be fine #fingers-crossed :)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D87774/new/
https://reviews.llvm.org/D87774
More information about the cfe-commits
mailing list