[libc-commits] [PATCH] D129918: [libc] Add utility classes for checking exceptional values.

[Siva Chandra via Phabricator via libc-commits]

sivachandra added a subscriber: jeffbailey.
sivachandra added inline comments.


================
Comment at: libc/src/__support/FPUtil/except_value_utils.h:42-45
+  //   output[i][0]: output bits corresponding to FE_TOWARDZERO
+  //   output[i][1]: offset for FE_UPWARD
+  //   output[i][2]: offset for FE_DOWNWARD
+  //   output[i][3]: offset for FE_TONEAREST
----------------
May be use a struct?

```
template <typename T> struct ExceptValueOuput {
  T rnd_towardzero_result;
  T rnd_upward_offset;
  T rnd_downward_offset;
  T rnd_nearest_offset;
}
```


================
Comment at: libc/src/__support/FPUtil/except_value_utils.h:54
+
+  static bool check(const ExceptionalValues &ExceptionalValues, UIntType x_bits,
+                    T &result) {
----------------
Because of the name like `check`, I was thinking this is a test utility. May be call it `expect_value_output` or something like that?

Also, @jeffbailey is working on an equivalent of `std::optional`: https://reviews.llvm.org/D129920. It seems like using it here would be appropriate? 

Lastly, does it have to be a static method - can it not be a non-static method? Then, you can name the method just `get_output`?


================
Comment at: libc/src/__support/FPUtil/except_value_utils.h:57
+    for (int i = 0; i < N; ++i) {
+      if (unlikely(x_bits == ExceptVals.inputs[i])) {
+        UIntType out_bits = ExceptVals.outputs[i][0]; // FE_TOWARDZERO
----------------
I still see typos and improper naming conventions - for example, I think you want to using `ExceptionalValues` here and not `ExceptValues`? Which makes wonder if this is being sufficiently tested? If not in use, just remove it? 


================
Comment at: libc/src/__support/FPUtil/except_value_utils.h:78
+
+  static bool check_odd_func(const ExceptionalValues &ExceptVals,
+                             UIntType x_abs, bool sign, T &result) {
----------------
Can you add documentation about this method so that it is clear as to why it there at all?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129918