[STLExtras] Add size() for arrays

[jhenderson]

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).

[jhenderson]

In D49402#1164683, @jhenderson wrote:

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]

Added Jame's suggestions

[jhenderson]

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).

[paulsemel]

include/llvm/ADT/STLExtras.h

1041

Ok I will add it in the comment ::)

[vsk]

Thanks, LGTM.

[efriedma]

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

[paulsemel]

In D49402#1165541, @efriedma wrote:

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]

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.

[jhenderson]

In D49402#1165541, @efriedma wrote:

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]

I agree with James !

[dblaikie]

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?

include/llvm/ADT/STLExtras.h

auto size(R &&Range, typename std::enable_if<
                          std::is_same<typename std::iterator_traits<decltype(
                                           Range.begin())>::iterator_category,
                                       std::random_access_iterator_tag>::value,
                          void>::type * = nullptr)
     -> decltype(std::distance(Range.begin(), Range.end())) {
   return std::distance(Range.begin(), Range.end());
 }
 
+template <class T, std::size_t N>
+constexpr size_t size(const T (&array)[N]) noexcept {
+  return N;
+}
+
 //===----------------------------------------------------------------------===//
 //     Extra additions to <memory>
 //===----------------------------------------------------------------------===//
 
 // Implement make_unique according to N3656.
 
 /// Constructs a `new T()` with the given args and returns a
 ///        `unique_ptr<T>` which owns the object.

unittests/ADT/STLExtrasTest.cpp

TEST(STLExtrasTest, ADLTest) {
   EXPECT_EQ(s.swap_val, "lhs");
   EXPECT_EQ(s2.swap_val, "rhs");
 
   int count = 0;
   llvm::for_each(s, [&count](int) { ++count; });
   EXPECT_EQ(5, count);
 }
 
+TEST(STLExtrasTest, ArraySize) {
+  int a1[] = {1, 2, 3, 4};
+
+  EXPECT_EQ(size(a1), 4);
+}
+
 } // namespace