awarzynski added a comment. Thanks for adding this, Diana! :)
Overall, makes sense to me. I have a couple of questions/points. In D126291#3547023 <https://reviews.llvm.org/D126291#3547023>, @rovka wrote: > With this patch in place, we still need to add `-Xlinker -subsystem:console' > in order to compile a simple 'Hello World' to run from the console. Is this behavior consistent with "clang.exe"? Also, any clue why is this needed? Some documentation here <https://docs.microsoft.com/en-us/cpp/build/reference/subsystem-specify-subsystem?view=msvc-170>. I've scanned it and am no wiser :/ > I tried to use --ld-path=various/paths instead, but it seems to be ignored on > Windows (I even get a warning about "argument unused during compilation"). If > anyone has any ideas about how to make the test more robust, please let me > know. From what I can see, `--ld-path` is parsed inside ToolChain::GetLinkerPath <https://github.com/llvm/llvm-project/blob/e601b2a1542710789395ab1121b1ccc7076e39d1/clang/lib/Driver/ToolChain.cpp#L597>. This method **is used** in Gnu.cpp <https://github.com/llvm/llvm-project/blob/e601b2a1542710789395ab1121b1ccc7076e39d1/clang/lib/Driver/ToolChains/Gnu.cpp#L690>, but not in MSVC.cpp. This is consistent with what you've observed, i.e. that `--ld-path` is ignored on Windows. I'm not aware of an alternative. ================ Comment at: flang/test/Driver/linker-flags-windows.f90:5-6 + +! NOTE: The additional linker flags tested here are currently specified in +! clang/lib/Driver/Toolchains/MSVC.cpp. +! REQUIRES: system-windows ---------------- [nit] Would you mind updating [[ https://github.com/llvm/llvm-project/blob/e601b2a1542710789395ab1121b1ccc7076e39d1/flang/test/Driver/linker-flags.f90#L5-L9 | linker-flags.f90 ]] so that the comments in both files are more consistent? In particular, this patch makes the following comment redundant: > If you are running this test on a yet another platform and it is failing for > you, please either update the following or (preferably) update the linker > invocation accordingly. I added it there as a hint for people adding support for platforms inconsistent with Unix. Having a dedicated Unix and Windows tests is better :) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126291/new/ https://reviews.llvm.org/D126291 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
