This is an archive of the discontinued LLVM Phabricator instance.

ADT/ArrayRef: Add makeMutableArrayRef overloads
ClosedPublic

Authored by nhaehnle on May 25 2022, 1:46 PM.

Details

Summary

Equivalent overloads already exist for makeArrayRef.

Diff Detail

Event Timeline

nhaehnle created this revision.May 25 2022, 1:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 25 2022, 1:46 PM
nhaehnle requested review of this revision.May 25 2022, 1:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 25 2022, 1:46 PM
efriedma added inline comments.May 25 2022, 1:49 PM
llvm/include/llvm/ADT/ArrayRef.h
549

Missing doc comment?

llvm/unittests/ADT/MutableArrayRefTest.cpp
18 ↗(On Diff #432120)

This comment doesn't really make sense.

MaskRay added inline comments.May 25 2022, 2:06 PM
llvm/unittests/ADT/MutableArrayRefTest.cpp
19 ↗(On Diff #432120)

You may reuse ArrayRefTest.cpp since we use ArrayRef.h instead of MutableArrayRef.h.

There should be a test checking that the element is mutable.

nhaehnle updated this revision to Diff 432340.May 26 2022, 11:29 AM

Address reviews:

  • fix comments
  • moved to ArrayRefTest.cpp
  • add a check that the mutable array ref elements can be assigned to
MaskRay added inline comments.May 26 2022, 11:45 AM
llvm/include/llvm/ADT/ArrayRef.h
549

Adding a comment for each makeMutableArrayRef overload doesn't add much value to me. The argument types are obvious.

I think having one comment covering all the overloads suffices.

Ping -- are there any substantial comments left? Based on previous feedback this is probably good to go with the changes I made last week?

There are two comments which are "Not Done". You need to address them and click "Done".

nhaehnle marked 2 inline comments as done.Jun 1 2022, 1:27 PM
nhaehnle added inline comments.
llvm/include/llvm/ADT/ArrayRef.h
549

This just feels like a matter of taste. The patch as-is follows the existing convention in the file.

nhaehnle marked an inline comment as done.Jun 4 2022, 6:19 AM

Ping

kuhar accepted this revision.Jun 5 2022, 11:48 AM
kuhar added a subscriber: kuhar.

LGTM

This revision is now accepted and ready to land.Jun 5 2022, 11:48 AM
This revision was landed with ongoing or failed builds.Jun 9 2022, 1:00 AM
This revision was automatically updated to reflect the committed changes.