This is an archive of the discontinued LLVM Phabricator instance.

[ADT] Fix const-correctness issues in `zippy`
ClosedPublic

Authored by kuhar on Feb 26 2023, 3:43 PM.

Details

Summary

This defines the iterator tuple based on the storage type of zippy,
instead of its type arguments. This way, we can support temporaries that
gets passed in and allow for them to be modified during iteration.

Because the iterator types to the tuple storage can have different types
when the storage is and isn't const, this defines a const iterator type
and non-const begin/end functions. This way we avoid unintentional
casts, e.g., trying to cast vector<bool>::reference to
vector<bool>::const_reference, which may be unrelated types that are
not convertible.

This patch is a general and free-standing improvement but my primary use
is in the implemention a version of enumerate that accepts multiple ranges:
D144583.

Diff Detail

Event Timeline

kuhar created this revision.Feb 26 2023, 3:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 26 2023, 3:43 PM
Herald added a subscriber: hanchung. · View Herald Transcript
kuhar requested review of this revision.Feb 26 2023, 3:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 26 2023, 3:43 PM
kuhar updated this revision to Diff 500633.Feb 26 2023, 3:44 PM

Drop unnecessary headers

zero9178 accepted this revision.Feb 27 2023, 3:32 AM

LGTM, but please give it a day or two to give other reviewers a chance to comment as well

This revision is now accepted and ready to land.Feb 27 2023, 3:32 AM
dblaikie added inline comments.Feb 27 2023, 5:08 PM
llvm/unittests/ADT/IteratorTest.cpp
453–456

These could be static_asserts instead, perhaps? (similarly below)

502–503

Does this code work? How? Wouldn't the lifetimes of the temporary ranges expire before the body of the loop runs?

kuhar updated this revision to Diff 500989.Feb 27 2023, 5:51 PM
kuhar marked 2 inline comments as done.

Use static_assert to test for constness. Rebase.

llvm/unittests/ADT/IteratorTest.cpp
502–503

These ranges get moved into the tuple<...> storage; inside zippy. From then on, we can use references obtained from this storage to access them. So this should not rely on any lifetime extensions on the temporaries passed to zip_equal.

kuhar updated this revision to Diff 500991.Feb 27 2023, 5:57 PM

Add comment explaining lifetimes of temporaries in the ZipEqualTemporaries test.

kuhar updated this revision to Diff 501003.Feb 27 2023, 7:36 PM

Fix the definition of MakeConst to return values instead of references, to avoid dangling references.

kuhar added inline comments.Feb 27 2023, 7:47 PM
llvm/unittests/ADT/IteratorTest.cpp
502–503

To confirm, I ran this under ASan and UBSan and they don't report anything once I fixed the definition of MakeConst

dblaikie accepted this revision.Feb 28 2023, 1:18 PM

Might've been nice to separate the adl_* support from the other work in this patch, but at least at a vague glance, it all looks good - thanks!

llvm/unittests/ADT/IteratorTest.cpp
502–503

ah, yeah, MakeConst especially made me uncertain this was correct - but yeah, returning by value, and zip carrying its arguments by value when passed temporaries makes sense that it all works together now, thanks!

May be worth a comment on MakeConst about its return value being const intentionally (though it's obvious enough from the name I suppose) - since const on by-value return types is usually a mistake & I could imagine a linter or something trying to clean that up.

kuhar updated this revision to Diff 501282.Feb 28 2023, 1:23 PM

Clarify the purpose of MakeConst

kuhar marked an inline comment as done.Feb 28 2023, 1:26 PM
This revision was landed with ongoing or failed builds.Feb 28 2023, 1:29 PM
This revision was automatically updated to reflect the committed changes.