awarzynski accepted this revision. awarzynski added a comment. This revision is now accepted and ready to land.
Thank you for working on this @FarisRehman ! This semantics implemented here are identical to what `f18` does. `-fdebug-instrumented-parse` from `f18` becomes `-fdebug-parsing-log` in `flang-new -fc1`. This was proposed (and accepted) in the RFC <https://lists.llvm.org/pipermail/flang-dev/2020-November/000588.html>. I've left a few comments inline, but nothing major and otherwise this is ready to land. ================ Comment at: flang/include/flang/Frontend/FrontendActions.h:18 +struct MeasurementVisitor { + template <typename A> bool Pre(const A &) { return true; } ---------------- This should be shared with `f18.cpp`, but currently we don't have a good place for that (and we probably want to understand how much is going to be shared first). So IMO it is fine to keep it here. Could you add a comment similar to [[ https://github.com/llvm/llvm-project/blob/adfd3c7083f9808d145239153c10f72eece485d8/flang/lib/Frontend/FrontendOptions.cpp#L29-L32 | this ]] so that it is clear that this should be moved at some point? ================ Comment at: flang/lib/Frontend/FrontendActions.cpp:313 + } else { + llvm::errs() << "Pre FIR Tree is NULL.\n"; + } ---------------- Could this use diagnostics instead? For example: ``` unsigned diagID = ci.diagnostics().getCustomDiagID(clang::DiagnosticsEngine::Error, "Pre FIR Tree is NULL."); ci.diagnostics().Report(diagID); ``` ================ Comment at: flang/test/Flang-Driver/debug-measure-parse-tree.f90:23-24 +!--------------------------------------- +! FRONTEND:18 objects +! FRONTEND:1496 total bytes + ---------------- The output for this test is: ``` Parse tree comprises 18 objects and occupies 1496 total bytes. ``` I think that the actual size (i.e. 18 and 1496) is not that relevant here and also prone to change (which may cause the test to become fragile). Instead, I suggest that we verify that `-fdebug-measure-parse-tree` indeed instructs the driver to measure the parse and print the result. So, perhaps this instead: ``` Parse tree comprises {{[0-9]+}} objects and occupies {{[0-9]+}} total bytes. ``` ================ Comment at: flang/test/Flang-Driver/debug-parsing-log.f90:23 +!--------------------------------------- +! FRONTEND:in the context: END PROGRAM statement + ---------------- IIUC, this output is not really specific to `-fdebug-parsing-log` (`-fsyntax-only` prints this too). I think that this would be more specific to this flag: ``` ! FRONTEN: fail 1 ! FRONTEN: fail 2 ! FRONTEN: fail 3 ``` This is still not great, but looking at the log itself I struggle to notice anything that would stand out. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96884/new/ https://reviews.llvm.org/D96884 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
