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