[PATCH] D49402: [STLExtras] Add size() for arrays

[James Henderson via Phabricator via llvm-commits]

jhenderson added a reviewer: vsk.
jhenderson added a subscriber: vsk.
jhenderson added a comment.

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



================
Comment at: include/llvm/ADT/STLExtras.h:1041
 
+template <class T, std::size_t N>
+constexpr size_t size(const T (&array)[N]) noexcept {
----------------
jhenderson wrote:
> 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!
Please add a comment, like the rest of the functions around here.


================
Comment at: include/llvm/ADT/STLExtras.h:1041-1042
 
+template <class T, std::size_t N>
+constexpr size_t size(const T (&array)[N]) noexcept {
+  return N;
----------------
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!


================
Comment at: unittests/ADT/STLExtrasTest.cpp:370
+
+  EXPECT_EQ(size(a1), 4);
+}
----------------
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).


Repository:
  rL LLVM

https://reviews.llvm.org/D49402