[PATCH] D106137: [flang][driver] Add support for Frontend Plugins

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

I think that this is almost ready to be merged. Just a couple more comments.



================
Comment at: flang/examples/CMakeLists.txt:1
 # This test is not run by default as it requires input.
 add_executable(external-hello-world
----------------
Could you add the following at the top of this file:
```
if(NOT FLANG_BUILD_EXAMPLES)
  set(EXCLUDE_FROM_ALL ON)
endif()
```
This will make sure that the examples are not build with e.g. `ninja` when `FLANG_BUILD_EXAMPLES` is `Off`.

In [[ https://github.com/llvm/llvm-project/blob/43e45f0ec920b45d6073c0aff47597c44948f52c/clang/examples/CMakeLists.txt#L1-L4 | Clang ]], CMake also contains 
```  set_property(DIRECTORY PROPERTY EXCLUDE_FROM_ALL ON)
```
I think that `set(EXCLUDE_FROM_ALL ON)` should be sufficient though (see [[ https://cmake.org/cmake/help/latest/prop_tgt/EXCLUDE_FROM_ALL.html#prop_tgt:EXCLUDE_FROM_ALL | CMake ]]  docs for `EXCLUDE_FROM_ALL`). 

Note  that in LLVM, there is [[ https://github.com/llvm/llvm-project/blob/43e45f0ec920b45d6073c0aff47597c44948f52c/llvm/cmake/modules/AddLLVM.cmake#L1264-L1273 | add_llvm_example_library ]]. It would be nice to use that, be we'd need to make sure that `LLVM_BUILD_EXAMPLES` is set correctly. But that's not required just now and could be added later.


================
Comment at: flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:147
+  // If there were errors in processing arguments, don't do anything else.
+  if (flang->diagnostics().hasErrorOccurred())
+    return false;
----------------
This will silence this warning:
```
if (flang->diagnostics().hasErrorOccurred()) {
    return false;
}
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106137/new/

https://reviews.llvm.org/D106137