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

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan added inline comments.


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:545
+    TODO(boxprochost.getLoc(), "fir.boxproc_host codegen");
+    return failure();
   }
----------------
awarzynski wrote:
> 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.
OK. I will retain, incase some compiler warns.


================
Comment at: flang/test/lit.cfg.py:38
 
+# ask llvm-config about asserts and build type
+llvm_config.feature_config(
----------------
awarzynski wrote:
> 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?
Thanks. Yes, this is a copy paste error, there is another flag (build-type) that we can query but as u point out, we do not use that here.


================
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'))
----------------
awarzynski wrote:
> [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?
Yes, I agree that `not_cmd ` is not very descriptive. Since this is specific to the todo messages, I am replacing it with `not_todo_cmd`. This gives a hint that a developer should use `not_todo_cmd` when todo messages are involved. This along with the description should hopefully be enough.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114371