[PATCH] D25342: Turn cl::values() (for enum) from a vararg function to using C++ variadic template

[Eric Fiselier via llvm-commits]

EricWF added a comment.

This LGTM. but I can't approve LLVM patches.



================
Comment at: llvm/include/llvm/Support/CommandLine.h:594-595
+/// as an initializer list to the ValuesClass constructor.
+template <typename... OptsTy> ValuesClass values(OptsTy... Options) {
+  return ValuesClass({Options...});
 }
----------------
mehdi_amini wrote:
> zturner wrote:
> > Is this going to give you unwanted copies?  Is there any difference if you take the parameter pack by rvalue reference?
> I expect copy-elision to kick-in anyway, but I can use universal reference as well.
Isn't moving a `OptionEnumValue`  equivalent to moving it anyway? 


================
Comment at: llvm/include/llvm/Support/CommandLine.h:583
+  ValuesClass(std::initializer_list<OptionEnumValue> Options)
+      : Values(std::move(Options)) {}
 
----------------
`std::initializer_list` is just a pair of pointers. Pass it by value.


================
Comment at: llvm/include/llvm/Support/CommandLine.h:594
+/// as an initializer list to the ValuesClass constructor.
+template <typename... OptsTy> ValuesClass values(OptsTy &&...Options) {
+  return ValuesClass({Options...});
----------------
It's weird to use forwarding references because we don't actually forward anything. I would capture either by value or by `const &`.


https://reviews.llvm.org/D25342