[flang-commits] [PATCH] D128262: [Fortran] Avoid digits in character constant

[Diana Picus via Phabricator via flang-commits]

rovka added a comment.

In D128262#3605337 <https://reviews.llvm.org/D128262#3605337>, @awarzynski wrote:

> In D128262#3604875 <https://reviews.llvm.org/D128262#3604875>, @rovka wrote:
>
>> From what I managed to google up, gfortran never introduces a newline, but ifort does unless told otherwise on the command line. As you can see, flang also introduces a newline by default.
>
> So, we know that the format of the output in this case is compiler-specific  and the reference output was generated using gfortran <https://github.com/llvm/llvm-test-suite/blob/25c31a4e8b54b5e2d4e03b21aa6631fdb248dc6e/Fortran/UnitTests/fcvs21_f95/CMakeLists.txt#L4-L13>. Generalizing it so that it works for other compilers makes sense to me.
>
>> think we should try to accept both. WDYT?
>
> How can we achieve this? Wouldn't that require a dedicated "reference" file for every compiler that we want to support here? Also, by replacing digits with letters you are basically making `fpcmp` ignore this particular bit of output, right? So:
>
> - why not delete this particular test line if it's to be ignored anyway?
> - why is the generated output only really verified with `fpcmp`?

No, it's not ignored, `fpcmp` handles strings correctly (it errors out if there are any differences). In this case, it only ignores the whitespaces because I'm telling it to <https://reviews.llvm.org/D128260>, but it still checks that the non-space characters are there.

> I've scanned llvm-test-suite and I don't see any obvious way to use any other DIFFPROGR <https://github.com/llvm/llvm-test-suite/blob/25c31a4e8b54b5e2d4e03b21aa6631fdb248dc6e/cmake/modules/SingleMultiSource.cmake#L124>. So it sounds like we can only use `fpcmp` for now and this tool shouldn't  be comparing strings, should it? Perhaps switching from SingleSource <https://github.com/llvm/llvm-test-suite/blob/25c31a4e8b54b5e2d4e03b21aa6631fdb248dc6e/cmake/modules/SingleMultiSource.cmake#L1> CMake logic in the test suite to e.g. TestFile <https://github.com/llvm/llvm-test-suite/blob/25c31a4e8b54b5e2d4e03b21aa6631fdb248dc6e/cmake/modules/TestFile.cmake> would help, but that's IMO outside the scope of this patch.

Yeah, definitely outside the scope of this patch :)


Repository:
  rT test-suite

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

https://reviews.llvm.org/D128262