This is an archive of the discontinued LLVM Phabricator instance.

[llvm][ADT] Add deduction guides for `MutableArrayRef`
ClosedPublic

Authored by jloser on Jan 9 2023, 2:40 PM.

Details

Summary

Similar to https://reviews.llvm.org/D140896, this adds deduction guides for the
counterpart of ArrayRef: MutableArrayRef. The update plan is the following:

  1. Add deduction guides for MutableArrayRef.
  2. Change uses in-tree from makeMutableArrayRef to use deduction guides
  3. Mark makeMutableArrayRef as deprecated for some time before removing to give downstream users time to update.

The deduction guides are similar to those provided by the makeMutableArrayRef
function templates, except we don't need one explicitly from MutableArrayRef.

Diff Detail

Event Timeline

jloser created this revision.Jan 9 2023, 2:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 9 2023, 2:40 PM
jloser requested review of this revision.Jan 9 2023, 2:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 9 2023, 2:40 PM
jloser updated this revision to Diff 487561.Jan 9 2023, 2:41 PM
jloser retitled this revision from [ADT] Add deduction guides for `MutableArrayRef` to [llvm][ADT] Add deduction guides for `MutableArrayRef`.

Add [llvm] to title

Test coverage might be nice - some static asserts about the type that's deduced is probably sufficient (static_assert(std::is_same_v<MutableArrayRef<int, 3>, MutableArrayRef(std::declval<int[3]>())>) or something like that for each deduction guide)?

jloser updated this revision to Diff 487626.Jan 9 2023, 5:56 PM

Add tests for deduction guides and runtime behavior

jloser added a comment.Jan 9 2023, 5:57 PM

Test coverage might be nice - some static asserts about the type that's deduced is probably sufficient (static_assert(std::is_same_v<MutableArrayRef<int, 3>, MutableArrayRef(std::declval<int[3]>())>) or something like that for each deduction guide)?

I added both compile time tests for each of the deduction guides and runtime tests similar to that of makeMutableArrayRef test above. Let me know what you think. Thanks for the nudge on testing.

LGTM

llvm/include/llvm/ADT/ArrayRef.h
590

I don't think this is going to be a n-op : it's going to call the copy constructor.

This revision is now accepted and ready to land.Jan 10 2023, 3:02 AM
jloser updated this revision to Diff 487768.Jan 10 2023, 5:54 AM
jloser edited the summary of this revision. (Show Details)

Remove deduction guide from MutableArrayRef: it's not needed. Add a test from both const and non-const MutableArrayRef to show there's no issues, or ambiguous deduction guides like before.

jloser added inline comments.Jan 10 2023, 5:56 AM
llvm/include/llvm/ADT/ArrayRef.h
590

Oops, sorry this was a copy-paste from the makeMutableArrayRef function template comments.

Turns out we don't actually need this since a const MutableArrayRef would bind with the first deduction guide and give back a MutableArrayRef<T>. I added an additional test to show we can deduce correctly from both a const and non-const MutableArrayRef input.

If this still looks good to you @serge-sans-paille, I'll land it. Thanks!

dblaikie accepted this revision.Jan 10 2023, 9:17 AM

I'd be OK with the testing just being the static_asserts without the rest of the testing (since that's all the deduction guide do - specifies what type to use, then the usual ctor rules kick in - and those ctors are already tested elsewhere) - but this is OK too, if you prefer it.

I'd be OK with the testing just being the static_asserts without the rest of the testing (since that's all the deduction guide do - specifies what type to use, then the usual ctor rules kick in - and those ctors are already tested elsewhere) - but this is OK too, if you prefer it.

I slightly prefer it actually (and it's what we do in libc++ too — both compile-time and runtime tests for testing deduction guides). Here, we plan on eventually removing makeMutableArrayRef (which tests runtime behavior above). When we remove those function templates and their tests, it'd be good to preserve similar behavior in my mind.

This revision was automatically updated to reflect the committed changes.