victorkingi added inline comments.
================
Comment at: flang/lib/Frontend/FrontendActions.cpp:976-1011
+ void
+ optimizationRemarkHandler(const llvm::DiagnosticInfoOptimizationBase &d) {
+ if (d.isPassed()) {
+ // Optimization remarks are active only if the -Rpass flag has a regular
+ // expression that matches the name of the pass name in \p d.
+ if (codeGenOpts.OptimizationRemark.patternMatches(d.getPassName()))
+ emitOptimizationMessage(
----------------
awarzynski wrote:
>
The if statement still needs to return if the pattern doesn't match, should I
leave it the way it is?
================
Comment at: flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:30
#include "llvm/Support/CommandLine.h"
+#include <clang/Basic/DiagnosticFrontend.h>
----------------
awarzynski wrote:
> No longer needed, right? Also, please use the the format consistent with the
> other `#include`s.
It's needed in the case of `clang::diag::warn_unknown_diag_option` in
`emitUnknownDiagWarning` function.
================
Comment at: flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:117
+
+void processWarningOptions(clang::DiagnosticsEngine &diags,
+ const clang::DiagnosticOptions &opts,
----------------
awarzynski wrote:
> awarzynski wrote:
> > ?
> I find this method quite confusing.
>
> 1. It doesn't process warning options - it processes remarks options (so the
> name is inaccurate).
> 2. It deals with `-Rpass -Rno-pass` cases (i.e. postive + negative flag), but
> that's normally done in CompilerInvocation when parsing other options. See
> e.g.
> https://github.com/llvm/llvm-project/blob/c52d9509d40d3048914b144618232213e6076e05/flang/lib/Frontend/CompilerInvocation.cpp#L161-L163.
> Why not use that logic instead?
> 3. It's been extracted from Clang - it would be very helpful to note that and
> highlight the difference between this method and similar method in Clang.
> 4. You can safely trim it to only deal with Remarks (so e.g. update `const
> clang::DiagnosticOptions &opts,` in the signature)
>
> Now, I am not requesting any major refactor here - I appreciate that e.g.
> these remark flags are defined a bit differently to other flags. But this
> method can be simplified and should be documented.
I found it difficult including it in `CompilerInvocation.cpp`
`parseCodeGenArgs` function since we are updating the DiagnosticsEngine object
belonging to CompilerInstance not the other. We would have to expose the
`DiagnosticsEngine` in `CompilerInvocation` class to do this. Would there be
another way?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D156320/new/
https://reviews.llvm.org/D156320
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
