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

Thu Sep 24 11:31:04 PDT 2020

sameeranjoshi added inline comments.

================
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:
> 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.
I think it's better to follow what `FIR`(fir-dev) and `llvm-project/flang` does.
They generally try to fully-qualify name of library and avoid using forward declarations.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87774