[PATCH] D114371: [Flang] Replace notifyMatchFailure with TODO hard failures

[Andrzej Warzynski via Phabricator via llvm-commits]

awarzynski accepted this revision.
awarzynski added a comment.

I've left a few more nits - feel free to ignore. Thanks for implementing this! I think that you've also addressed all of Mehdi's comments, but please give it another day before merging (in case we missed sth).



================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:545
+    TODO(boxprochost.getLoc(), "fir.boxproc_host codegen");
+    return failure();
   }
----------------
FYI, I tried removing `return failure();` and tested with
* `-DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=On`, and 
* `-DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=Off`.

No compiler errors/warnings in both cases. But it's probably safer to leave `return failure();` here.


================
Comment at: flang/test/lit.cfg.py:38
 
+# ask llvm-config about asserts and build type
+llvm_config.feature_config(
----------------
This will only check for "asserts", right? Build type affects whether "asserts" are enabled, but that's just an implementation detail here. At this stage we just want to know whether the `'asserts`' are "On", right?


================
Comment at: flang/test/lit.cfg.py:78
         unresolved='fatal')]
 
+if 'asserts' in config.available_features:
----------------
[nit] It would be helpful to have a comment explaining "why" we may need `not` with `--crash` (and, ultimately, `%not_cmd`).


================
Comment at: flang/test/lit.cfg.py:80
+if 'asserts' in config.available_features:
+    tools.append(ToolSubst('%not_cmd', command=FindTool('not'), extra_args=['--crash'],
+        unresolved='fatal'))
----------------
[nit] Wouldn't `%not_or_crash` be more descriptive than `%not_cmd`? Both `not` and `not --crash` are "commands", but:
```
  //   not cmd
  //     Will return true if cmd doesn't crash and returns false.
  //   not --crash cmd
  //     Will return true if cmd crashes (e.g. for testing crash reporting).
```
(from [[ https://github.com/llvm/llvm-project/blob/main/llvm/utils/not/not.cpp#L8-L12 | not.cpp ]]). But perhaps that's more confusing?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114371