awarzynski accepted this revision. awarzynski added a comment. This revision is now accepted and ready to land.
I've left a few comments, but these are nice-to-haves from my perspective. @tskeith , what do you think about the suggested changes in `f18`? We could upload a separate patch for that. ================ Comment at: flang/test/Flang-Driver/fdefault.f90:4 + +! REQUIRES: new-flang-driver + ---------------- tskeith wrote: > Can't this work with the f18 driver too? That's the best way to ensure they > are consistent. I think that this is a good idea, but there are two things that need to be addressed first: **OPTION NAMES** `f18` uses `-module` rather than `-module-dir`. I suggest adding an alias, `-module-dir`, to `f18` (e.g. similarly to how `-fparse-only` and `-fsyntax-only` are handled) **IMPLEMENTATION** If I understand correctly, the current implementation of `-fdefault-real-8` in `f18` is incomplete, see [[ https://github.com/llvm/llvm-project/blob/b6db47d7e044730dc3c9b35dae6697eee0885dbf/flang/tools/f18/f18.cpp#L568-L569 | here ]]: ``` } else if (arg == "-r8" || arg == "-fdefault-real-8") { defaultKinds.set_defaultRealKind(8); ``` I think that there should be `defaultKinds.set_defaultRealKind(16);` as well. @tskeith could you confirm? ================ Comment at: flang/test/Flang-Driver/fdefault.f90:10 +! RUN: not %flang-new -fsyntax-only -fdefault-double-8 %s 2>&1 | FileCheck %s --check-prefix=DOUBLE +! RUN: mkdir -p %t/dir-flang-new && %flang-new -fsyntax-only -module-dir %t/dir-flang-new -fdefault-real-8 %s 2>&1 +! RUN: cat %t/dir-flang-new/m.mod | grep 'real_kind=8_4' ---------------- tskeith wrote: > awarzynski wrote: > > tskeith wrote: > > > I don't think you need to create a subdirectory. `%t` is a temp directory > > > specific to this test. > > > > > > So I'm suggesting: > > > ``` > > > ! RUN: %flang -fsyntax-only -fdefault-real-8 %s > > > ``` > > From [[ https://llvm.org/docs/CommandGuide/lit.html#substitutions | LIT > > docs ]]: > > > > ``` > > %t temporary file name unique to the test > > %T parent directory of %t (not unique, deprecated, do not use) > > ``` > > > > So, IIUC, we do need to create a directory. We could skip `<dir-flang-new>` > > in the directory name, but it does no harm and can be really helpful when > > parsing CI logs. > > > > I might be missing something though. @tskeith ? > I thought lit created an empty directory for `%t` but apparently not. You > just get whatever was left over from last run. So the safest thing to do is > this for each compilation command: > `! RUN: rm -rf %t && mkdir %t && %flang -module-dir %t ...` > > It's true that adding an extra subdirectory does no harm besides clutter, but > what's the benefit? `%t` is normally used as a temporary file name. By using e.g. `%t/dir-flang-new` instead, we communicate it clearly that we are using it as a directory name. This is obvious now (and may seem unnecessary), but it might be less so in 6 months or when somebody works on this project (or particular test) in the future. Either way, this test is functionally correct and in fact incredibly helpful. Both `%t` and `%t/dir-flang-new` will work perfectly fine. ================ Comment at: flang/test/Flang-Driver/flarge_sizes.f90:1 +! Ensure argument -flarge-sizes works as expected. +! TODO: Add checks when actual codegen is possible for this family ---------------- Could you add a `NOOPTION` variant like you did in fdefault.f90? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96344/new/ https://reviews.llvm.org/D96344 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
