This is an archive of the discontinued LLVM Phabricator instance.

[libc] Add utility classes for checking exceptional values.
AbandonedPublic

Authored by lntue on Jul 15 2022, 8:33 PM.

Details

Summary

This is refactored from https://reviews.llvm.org/D123154.

Diff Detail

Event Timeline

lntue created this revision.Jul 15 2022, 8:33 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 15 2022, 8:33 PM
lntue requested review of this revision.Jul 15 2022, 8:33 PM
sivachandra added inline comments.Jul 18 2022, 10:40 AM
libc/src/__support/FPUtil/except_value_utils.h
20

Can you please add documentation about the need and how to use this class?

41

What is x_abs here?

lntue updated this revision to Diff 445603.Jul 18 2022, 12:46 PM

Address comments.

lntue updated this revision to Diff 445609.Jul 18 2022, 12:58 PM
lntue marked an inline comment as done.

Update comments.

lntue marked an inline comment as done.Jul 18 2022, 12:58 PM
lntue added inline comments.
libc/src/__support/FPUtil/except_value_utils.h
41

Oops, copy-paste error! Fixed.

sivachandra added inline comments.
libc/src/__support/FPUtil/except_value_utils.h
43–46

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;
}
55

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?

58

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?

79

Can you add documentation about this method so that it is clear as to why it there at all?

lntue updated this revision to Diff 446027.EditedJul 19 2022, 8:24 PM

Use cpp::Optional from https://reviews.llvm.org/D129920 as return type.

lntue marked 2 inline comments as done.Jul 19 2022, 8:30 PM
lntue added inline comments.
libc/src/__support/FPUtil/except_value_utils.h
55

I updated the patch to use cpp::Optional from @jeffbailey patch https://reviews.llvm.org/D129920, and change these to standalone methods.

jeffbailey added inline comments.Jul 19 2022, 8:49 PM
libc/src/__support/FPUtil/except_value_utils.h
55

Since the interface is from the C++ standard and I know the thing works based on our tz variable parsing tests, I could commit what I have in that patch and then do the changes that I want to do for better conformance in a follow-up patch.

lntue abandoned this revision.Sep 8 2022, 10:03 PM