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

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski marked 4 inline comments as done.
awarzynski added a comment.

In D87774#2293283 <https://reviews.llvm.org/D87774#2293283>, @sameeranjoshi wrote:

> Do you know if there are any bots configured to handle out-of-tree changes?
> That might be helpful to avoid configuration differences and test OFT patches.

I'm not aware of a buildbot that would test out-of-tree builds. I also don't know how hard it would be to set one up (AFAIK, there's no precedence of out-of-tree LLVM buildbots). Defending every sub-project/feature that people care about with a buildbot is important. Sadly I won't have the bandwidth to set one up for out-of-tree builds any time soon. This my gentle call for volunteers :)



================
Comment at: flang/lib/Frontend/TextDiagnosticPrinter.cpp:24
+
+TextDiagnosticPrinter::TextDiagnosticPrinter(
+    raw_ostream &os, clang::DiagnosticOptions *diags)
----------------
kiranchandramohan wrote:
> awarzynski wrote:
> > 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?
> I think in flang the style is to declare the class in the source file if it is only used in that file. If it is used elsewhere then put it in the header. If it is used in another directory then move the header to include directory.
This class is used in multiple files:

* flang/lib/Frontend/CompilerInstance.{cpp|h}
* flang/tools/flang-driver/driver.cpp
* flang/unittests/Frontend/CompilerInstanceTest.cpp

IIUC, this is now resolved. 


================
Comment at: flang/tools/flang-driver/driver.cpp:14
 #include "clang/Driver/Driver.h"
+#include "flang/Frontend/CompilerInvocation.h"
+#include "flang/Frontend/TextDiagnosticPrinter.h"
----------------
CarolineConcatto wrote:
> Why do we need invocation here?
Required for ParseDiagnosticArgs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87774