awarzynski added a comment. @stuartellis , thank you for working on this! I've left a few comments - mostly nits.
================ Comment at: flang/examples/HelloWorld/CMakeLists.txt:1-5 +add_llvm_library( + flangHelloWorldPlugin + MODULE + HelloWorldPlugin.cpp +) ---------------- I think that this should be fine as is, but will not work on Windows. Check this example: [[ https://github.com/llvm/llvm-project/blob/main/clang/examples/PrintFunctionNames/CMakeLists.txt | PrintFunctionNames ]]. I think that it's fine not to support Windows for now, but we should document this. Also, your tests need to marked as Linux-only (e.g. with `// REQUIRES: shell` or something similar). ================ Comment at: flang/examples/HelloWorld/HelloWorldPlugin.cpp:1 +#include "flang/Frontend/FrontendActions.h" +#include "flang/Frontend/FrontendPluginRegistry.h" ---------------- Missing LLVM file header: https://llvm.org/docs/CodingStandards.html#file-headers ================ Comment at: flang/include/flang/Frontend/FrontendPluginRegistry.h:27 +#endif // LLVM_FLANG_FRONTEND_FRONTENDPLUGINREGISTRY_H \ No newline at end of file ---------------- FIXME ================ Comment at: flang/lib/Frontend/CompilerInvocation.cpp:202 + if (llvm::opt::Arg *a = args.getLastArg(clang::driver::options::OPT_load)) { + opts.plugins.push_back(a->getValue()); ---------------- [nit] Perhaps worth adding a comment to highlight _which_ option is being parsed? ================ Comment at: flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:87 + if (plugin.getName() == ci.frontendOpts().ActionName) { + std::unique_ptr<PluginParseTreeAction> P(plugin.instantiate()); + return std::move(P); ---------------- Lower case `P` ================ Comment at: flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:92 + unsigned diagID = ci.diagnostics().getCustomDiagID( + clang::DiagnosticsEngine::Error, "unable to find plugin '%0'"); + ci.diagnostics().Report(diagID) << ci.frontendOpts().ActionName; ---------------- Could you add a test for this diagnostic? ================ Comment at: flang/test/Driver/plugin-example.f90:7 +! RUN: %flang_fc1 -load %llvmshlibdir/flangHelloWorldPlugin%pluginext -plugin -hello-world %s 2>&1 | FileCheck %s +! CHECK: Hello World from your new plugin ---------------- [tiny nit] Could you make it Flang specific? E.g. `Hello World from your new Flang plugin` ================ Comment at: flang/test/lit.cfg.py:55 +# Plugins (loadable modules) +if config.has_plugins and config.llvm_plugin_ext: + config.available_features.add('plugins') ---------------- Could `llvm_plugin_ext` be ever empty? Perhaps I'm missing something, but it feels that `if config.has_plugins` should be sufficient here. ================ Comment at: flang/tools/flang-driver/CMakeLists.txt:30 +export_executable_symbols_for_plugins(flang-new) + ---------------- We should make it possible to turn this off, perhaps: ```lang=clamg # Some clever comment if(FLANG_PLUGIN_SUPPORT) export_executable_symbols_for_plugins(flang-new) endif() ``` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D106137/new/ https://reviews.llvm.org/D106137 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
