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