This is an archive of the discontinued LLVM Phabricator instance.

Turn cl::values() (for enum) from a vararg function to using C++ variadic template
ClosedPublic

Authored by mehdi_amini on Oct 6 2016, 12:50 PM.

Event Timeline

mehdi_amini retitled this revision from to Turn cl::values() (for enum) from a vararg function to using C++ variadic template.
mehdi_amini updated this object.
mehdi_amini added reviewers: joerg, zturner.
mehdi_amini added a subscriber: llvm-commits.
zturner added inline comments.Oct 6 2016, 2:09 PM
llvm/include/llvm/Support/CommandLine.h
585–589

Probably should change this to a range-based for.

594–595

Is this going to give you unwanted copies? Is there any difference if you take the parameter pack by rvalue reference?

mehdi_amini added inline comments.Oct 6 2016, 2:11 PM
llvm/include/llvm/Support/CommandLine.h
594–595

I expect copy-elision to kick-in anyway, but I can use universal reference as well.

mehdi_amini updated this revision to Diff 73850.Oct 6 2016, 2:13 PM

Use for-range loop and universal reference.

EricWF added a subscriber: EricWF.Oct 8 2016, 2:29 AM

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

llvm/include/llvm/Support/CommandLine.h
583

std::initializer_list is just a pair of pointers. Pass it by value.

594

It's weird to use forwarding references because we don't actually forward anything. I would capture either by value or by const &.

594–595

Isn't moving a OptionEnumValue equivalent to moving it anyway?

EricWF added inline comments.Oct 8 2016, 3:00 AM
llvm/include/llvm/Support/CommandLine.h
594–595

Isn't moving a OptionEnumValue equivalent to copying it anyway?

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

Don't be so shy :)

I put it up for review because I wasn't sure about the variadic template, if there was something more straightforward to build the initializer list and/or initialize the vector. I trust that if it is fine to you, it can be committed :)

This revision was automatically updated to reflect the committed changes.