richard.barton.arm requested changes to this revision.
richard.barton.arm added a comment.
A few specific comments to address here as well as the pre-commit linting ones.
================
Comment at: clang/lib/Driver/Driver.cpp:1584
void Driver::PrintVersion(const Compilation &C, raw_ostream &OS) const {
+ if (IsFlangMode()) {
+ OS << "Flang experimental driver (flang-new)" << '\n';
----------------
Instead of this early exit, could we instead of calling getClangFullVersion
below call getClangToolFullVersion with a different string here?
```
if (IsFlangMode())
OS >> getClangToolFullVersion("flang-new") << '\n';
else
OS >> getClangFullVersion();
```
That way, we get the full clang version screen already implemented 'for free'
and the code is nicer too (IMO)
================
Comment at: flang/CMakeLists.txt:20
option(LINK_WITH_FIR "Link driver with FIR and LLVM" ON)
+option(BUILD_FLANG_NEW_DRIVER "Build the flang driver frontend" OFF)
----------------
Generally we should make sure all Flang-specific CMake build configuration
variables start with FLANG. Suggest this is FLANG_BUILD_NEW_DRIVER or similar.
================
Comment at: flang/CMakeLists.txt:74
+
if(LINK_WITH_FIR)
----------------
Remove
================
Comment at: flang/test/lit.cfg.py:40
+# exclude the tests for flang_new driver while there are two drivers
+if config.include_flang_new_driver_test == "OFF":
+ config.excludes = ['Inputs', 'CMakeLists.txt', 'README.txt', 'LICENSE.txt',
'Flang-Driver']
----------------
I think it would be cleaner to define config.excludes unconditionally then
append the Flang-Driver dir if our condition passes.
================
Comment at: flang/test/lit.cfg.py:64
tools = [
+ ToolSubst('%flang-new', command=FindTool('flang-new'), unresolved='ignore'),
ToolSubst('%f18', command=FindTool('f18'),
----------------
Rather than always trying to add flang-new and ignoring failure, I think it
would be better to conditionally add this to `tools` iff we are building the
new driver and so have `config.include_flang_new_driver_test = "ON"`. This way
if you are building the new driver and for some reason lit fails to resolve the
tool then you get a nice error before trying to run tests on a binary that is
not there.
================
Comment at: flang/test/lit.site.cfg.py.in:13
+# controld the regression test for flang-new driver
+config.include_flang_new_driver_test="@BUILD_FLANG_NEW_DRIVER@"
----------------
typo "controld"
================
Comment at: flang/unittests/CMakeLists.txt:27
+if (BUILD_FLANG_NEW_DRIVER)
+add_subdirectory(Frontend)
+endif()
----------------
indentation?
================
Comment at: llvm/lib/Option/OptTable.cpp:621
+ // If `Flags` is empty (i.e. it's an option without any flags) then this is
+ // a Clang-only option. If:
+ // * we _are not_ in Flang Mode (FlagsToExclude contains FlangMode), then
----------------
This is not the sort of change I expect to see in an llvm support library given
that it seems very clang and flang specific.
I think this needs to be re-written to be more generic, perhaps tweaking the
interface to be able to express the logic you want to use for flang and clang.
Why is it not sufficient to call this function unchanged from both flang and
clang but with a different set of FlagsToInclude/FlagsToExclude passed in using
this logic to set that on the clang/flang side?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D86089/new/
https://reviews.llvm.org/D86089
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
