[flang-commits] [PATCH] D126291: [flang][Driver] Update link job on windows

[Diana Picus via Phabricator via flang-commits]

rovka added inline comments.


================
Comment at: flang/test/Driver/linker-flags.f90:51
+! MSVC-NOT: libcmt
+! MSVC-NOT: oldnames
+! MSVC-SAME: "[[object_file]]"
----------------
awarzynski wrote:
> rovka wrote:
> > awarzynski wrote:
> > > rovka wrote:
> > > > mmuetzel wrote:
> > > > > Lines 50-51 seem to be duplicates of lines 44-45. Is this intentional?
> > > > Yes, I don't want those to appear either before or after the Fortran libs. I guess if we wanted to be pedantic we'd also check that they don't appear after the object_file, or between the libs and the subsystem, but that seems a bit much.
> > > Based on the [[ https://llvm.org/docs/CommandGuide/FileCheck.html#the-check-same-directive | docs ]], I'd say that this would be the idiomatic way to do this:
> > > ```lang=bash
> > > ! MSVC-LABEL:
> > > ! MSVC-NOT: 
> > > ! MSVC-SAME:
> > > ```
> > > IIUC, the following would only be needed if there's a potential for `libcmt` or `oldnames` to appear on a separate line:
> > > ```
> > > ```lang=bash
> > > ! MSVC-LABEL:
> > > ! MSVC-NOT: 
> > > ! MSVC-SAME:
> > > ! MSVC-NOT:
> > > ```
> > > But this wouldn't happen, right? (there's going to be only one linker invocation). Also, you could just use [[ https://llvm.org/docs/CommandGuide/FileCheck.html#options | --implicit-check-not ]] :)
> > > Based on the [[ https://llvm.org/docs/CommandGuide/FileCheck.html#the-check-same-directive | docs ]], I'd say that this would be the idiomatic way to do this:
> > > ```lang=bash
> > > ! MSVC-LABEL:
> > > ! MSVC-NOT: 
> > > ! MSVC-SAME:
> > > ```
> > 
> > Based on the same docs, I would say it shouldn't be enough to mention it just once. But that's just what I expect, the docs are completely unhelpful about the actual behaviour here. For instance, I would expect to be able to write
> > ```
> > ! MSVC-NOT: should-only-come-after-X
> > ! MSVC-SAME: X
> > ```
> > If the MSVC-NOT applies to the whole line, then lines with 'X should-come-after-X' get rejected, but imo they should be accepted (I'll admit I didn't actually verify this, and anyway the implicit-check-not makes the whole discussion moot).
> > 
> > > IIUC, the following would only be needed if there's a potential for `libcmt` or `oldnames` to appear on a separate line:
> > > ```
> > > ```lang=bash
> > > ! MSVC-LABEL:
> > > ! MSVC-NOT: 
> > > ! MSVC-SAME:
> > > ! MSVC-NOT:
> > > ```
> > 
> > I agree that if there's no MSVC-SAME after the last MSVC-NOT, then the MSVC-NOT would apply to the following line. 
> > 
> > > But this wouldn't happen, right? (there's going to be only one linker invocation). Also, you could just use [[ https://llvm.org/docs/CommandGuide/FileCheck.html#options | --implicit-check-not ]] :)
> > 
> > Wooow 😍 I didn't know about that one, I'll definitely update the test to use it, thanks! 
> > Based on the same docs, I would say it shouldn't be enough to mention it just once. 
> 
> I'm re-rereading the docs and agree. Sounds like `--implicit-check-not` is the best option.
> 
> > For instance, I would expect to be able to write
> > ```
> > ! MSVC-NOT: should-only-come-after-X
> > ! MSVC-SAME: X
> > ```
> > If the MSVC-NOT applies to the whole line, then lines with 'X should-come-after-X' get rejected
> 
> I think that it //should// and //is// accepted. Example below:
> 
> **file.f90**
> ```lang=bash
> ! RUN: cat %S/test.f90 | FileCheck %s
> 
> ! CHECK-LABEL: my-label
> ! CHECK-NOT: should-only-come-after-X
> ! CHECK-SAME: X
> ! CHECK-SAME: should-only-come-after-X
> ```
> 
> **test.f90**
> ```lang=bash
> my-label X should-only-come-after-X
> ```
> I copied the above into the Driver test dir and run like this:
> ```
> $ bin/llvm-lit -va ../../flang/test/Driver/file.f90
> -- Testing: 1 tests, 1 workers --
> PASS: Flang :: Driver/file.f90 (1 of 1)
> 
> Testing Time: 0.03s
>   Passed: 1
> ```
> 
> Does this agree with your experiments?
> 
> #filecheck-is-confusing :)
Yep, that's what I was saying :)


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

https://reviews.llvm.org/D126291