awarzynski added a comment.

Some high level comments:

- The logic in `parseCodeGenArgs` in CompilerInvocation.cpp is a bit complex 
and quite specialized - could you move it to a dedicated method?
- In a fair few places this patch make references to "diagnostic flags" or 
"diagnostic options". That's borrowed from Clang where there's one code path 
for `-W<opt>` and `-R<opt>` flags. Note that in Clang the logic for `-W<opt>` 
is vastly more complex. This is completely absent in Flang. So, everything 
that's added here is added specifically for "remarks" and it would be helpful 
to be explicit about that.  For example, `processWarningOptions` is misleading. 
`processRemakrsOptions` would be more accurate.

Thank you :)



================
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:227
 
+  // Specifies what passes to include. If not provided
+  // -fsave-optimization-record will include all passes.
----------------
> // Specifies what passes to include.

Could you be more specific, what passes are you referring to? Included where?


================
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:242
+
+  // Specify which missed passes should be reported using a regex.
+  opts.OptimizationRemarkMissed = parseOptimizationRemark(
----------------
>   // Specify which missed passes should be reported using a regex.

Reported where?


================
Comment at: flang/lib/Frontend/TextDiagnosticPrinter.cpp:37
+// Print any diagnostic option information to a raw_ostream.
+static void printDiagnosticOptions(llvm::raw_ostream &os,
+                                   clang::DiagnosticsEngine::Level level,
----------------
victorkingi wrote:
> awarzynski wrote:
> > Why is this method needed and can it be tested?
> This method prints out the pass that was done e.g. [-Rpass=inline ], 
> [-Rpass=loop-delete] next to the optimization message.
> It is tested by the full remark message emitted test in 
> flang/test/Driver/optimization-remark.f90
Flang has very _very_ limited support for warning flags. So this is going to be 
used specifically for Remarks. Please update and document accordingly.


================
Comment at: flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:30
 #include "llvm/Support/CommandLine.h"
+#include <clang/Basic/DiagnosticFrontend.h>
 
----------------
No longer needed, right? Also, please use the the format consistent with the 
other `#include`s.


================
Comment at: flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:117
+
+void processWarningOptions(clang::DiagnosticsEngine &diags,
+                           const clang::DiagnosticOptions &opts,
----------------
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.


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

Reply via email to