Page MenuHomePhabricator

[STLExtras] Add size() for arrays
ClosedPublic

Authored by paulsemel on Jul 16 2018, 2:10 PM.

Details

Diff Detail

Repository
rL LLVM

Event Timeline

paulsemel created this revision.Jul 16 2018, 2:10 PM
jhenderson added a subscriber: vsk.

You might want to canvas opinions from others who have developed in this area recently, such as @vsk, before putting this in, but aside from the missing comment and couple of nits, this looks good.

I'd also make sure your commit comment mentions that this is intended to match std::size in C++17 (perhaps referencing the version you're implementing and the relevant version, i.e. version 2).

include/llvm/ADT/STLExtras.h
1041

Please add a comment, like the rest of the functions around here.

1041–1042

You're being inconsistent with your use of "std" here. Unfortunately, so is this entire file, so I can't say what is the preferred way, but at least make sure to use only one form in this function!

unittests/ADT/STLExtrasTest.cpp
370

It looks like it's more common to explicitly specify the llvm:: prefix here, and that would also help with replacing instances with std::size once LLVM allows C++17 code, via find/replace.

It might also be good to try two different test inputs (with e.g. a different size of array and type).

I'd also make sure your commit comment mentions that this is intended to match std::size in C++17 (perhaps referencing the version you're implementing and the relevant version, i.e. version 2).

Apologies, I messed up that last part - it should probably say "(perhaps referencing the implementation you are following)"

paulsemel updated this revision to Diff 155837.Jul 17 2018, 3:22 AM
paulsemel marked 3 inline comments as done.

Added Jame's suggestions

jhenderson accepted this revision.Jul 17 2018, 3:52 AM

LGTM. But I'd like somebody else to chip in on this as well before you commit. If nobody else does in the next couple of days, go ahead and commit.

include/llvm/ADT/STLExtras.h
1041

Thinking about it, you might want to say that this is identical to C++17's version (see similar comments around make_unique below).

This revision is now accepted and ready to land.Jul 17 2018, 3:52 AM
paulsemel added inline comments.Jul 17 2018, 5:10 AM
include/llvm/ADT/STLExtras.h
1041

Ok I will add it in the comment ::)

vsk accepted this revision.Jul 17 2018, 9:36 AM

Thanks, LGTM.

This appears to be identical to llvm::array_lengthof; do you plan to deprecate that function?

This appears to be identical to llvm::array_lengthof; do you plan to deprecate that function?

Indeed, this is the exact same function (and to be honest I didn't see it). But as c++17 is introducing this function with size as name, maybe we should switch to this one ? It's actually up to you guys ! :)

vsk added a comment.Jul 17 2018, 1:38 PM

Ah, I forgot about array_lengthof. If we were to add this overload, we ought to convert in-tree uses of array_lengthof to use it. If anyone's willing to tackle that, I'd say go for it.

This appears to be identical to llvm::array_lengthof; do you plan to deprecate that function?

Good catch! I agree with a simple swap from one to the other at some point, but actually, maybe we should just abandon this change, and use array_lengthof, until we are ready to use C++17 directly? It would make the find/replace much easier: I took a quick look, and it looks like array_lengthof is solely used for this particular function, and not some other invocation. But "size" on the other hand will be much harder to isolate.

paulsemel closed this revision.Jul 18 2018, 11:24 AM

I agree with James !

Seems like the existing size(R &&Range) function could be generalized (by
using adl_begin/end rather than member begin/end? Would that be enough) to
cover arrays as well, maybe?