This is an archive of the discontinued LLVM Phabricator instance.

[llvm] Add enum iteration to Sequence
ClosedPublic

Authored by gchatelet on Jul 19 2021, 7:38 AM.

Details

Summary

This patch allows iterating typed enum via the ADT/Sequence utility.

It also changes the original design to better separate concerns:

  • StrongInt only deals with safe intmax_t operations,
  • SafeIntIterator presents the iterator and reverse iterator interface but only deals with safe StrongInt internally.
  • iota_range only deals with SafeIntIterator internally.

    This design ensures that operations are always valid. In particular, "Out of bounds" assertions fire when:
    • the value_type is not representable as an intmax_t
    • iterator operations make internal computation underflow/overflow
    • the internal representation cannot be converted back to value_type

Diff Detail

Event Timeline

gchatelet created this revision.Jul 19 2021, 7:38 AM
gchatelet requested review of this revision.Jul 19 2021, 7:38 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript

This is a different take on D103900 which I had to revert because of the following UB operation.

[  DEATH   ] llvm-project/llvm/include/llvm/ADT/Sequence.h:198:68: runtime error: signed integer overflow: -9223372036854775808 - 1 cannot be represented in type 'long'

I think the new design is easier to reason about as all the casts and arithmetic operations are isolated and controlled.

gchatelet updated this revision to Diff 359795.Jul 19 2021, 8:11 AM
  • Use MathExtras functions instead of builtins for compiler compat
gchatelet updated this revision to Diff 359804.Jul 19 2021, 8:29 AM
  • Add documentation and remove unnecessary declarations
gchatelet updated this revision to Diff 359816.Jul 19 2021, 9:12 AM
  • Help readability by using operators instead of functino names
gchatelet updated this revision to Diff 359818.Jul 19 2021, 9:16 AM
  • Cleanup headers
courbet added inline comments.Jul 19 2021, 11:51 PM
llvm/include/llvm/ADT/Sequence.h
38–39

Is there any reason to make the dtor user-provided ? This makes the class non trivially copyable.

38–39

OverflowCheckingInt ?

43

use MathExtras's AddOverflow ?

55

so uintmax_t is no longer supported ? This should be noted somewhere. I'm wondering whether this is an issue or not...

228

This is no longer relevant, right ?

Also, add a note about signedness ?

gchatelet updated this revision to Diff 360090.Jul 20 2021, 5:13 AM
gchatelet marked 4 inline comments as done.
  • rebase
  • Change StrongInt to SafeInt
  • Add Begin and End value validity
gchatelet marked an inline comment as done.Jul 20 2021, 5:15 AM
gchatelet added inline comments.
llvm/include/llvm/ADT/Sequence.h
38–39

OverflowCheckingInt ?

I went with SafeInt as it's shorter and conveys the idea.

55

so uintmax_t is no longer supported ? This should be noted somewhere. I'm wondering whether this is an issue or not...

It is supported but not all of its values (no values above INTMAX_MAX are permitted).

I believe this is not a problem as this class is really about iterating values. It would be surprising to iterate values between INTMAX_MAX + 1 and UINTMAX_MAX.

gchatelet updated this revision to Diff 360094.Jul 20 2021, 5:22 AM
  • Fix documentation
gchatelet added inline comments.Jul 20 2021, 5:25 AM
llvm/include/llvm/ADT/Sequence.h
38–39

OverflowCheckingInt ?

I went with SafeInt as it's shorter and conveys the idea.

@courbet CheckedInt maybe?

courbet added inline comments.Jul 20 2021, 5:32 AM
llvm/include/llvm/ADT/Sequence.h
38–39

CheckedInt sounds good

gchatelet updated this revision to Diff 360096.Jul 20 2021, 5:42 AM
  • Rename SafeInt to CheckedInt
gchatelet marked an inline comment as done.Jul 20 2021, 5:42 AM
courbet accepted this revision.Jul 21 2021, 1:55 AM
This revision is now accepted and ready to land.Jul 21 2021, 1:55 AM
This revision was landed with ongoing or failed builds.Jul 21 2021, 5:49 AM
This revision was automatically updated to reflect the committed changes.