just my two cents.
Just to chime in a bit, I much prefer the pure-function style of returning both the min value and the associated index in a compound struct which is then unpacked with `auto [v, i] = ...`. I don't see why the function returning only the min value and the function returning both the min value and the index have to have the same name.
they don't have to, though having identical name seems more elegant, and friendly to tooling discovery such as ide intellisense. see glRotated* function series for a counter example. no overloading there results in an abundance of related yet different names just for pure syntactic need.
Now if people really have to pass a output parameter in order to return the index, my suggestion is to use `gsl::not_null`, i.e., a pointer that's guaranteed to be non-null. This way the semantics is clear. At usage cite, the "&" sign unequivocally indicates an output parameter. And you cannot pass a null without incurring assertion failure (for debug build at least).
in this particular instance, gsl::not_null is only a bandit, not a solution. there are other bad pointers, nullptr is only one kind. wild pointers, for example, do more harms. with nullptr, at least you get an assertion or exception in runtime and notice the bug right away. with wild pointers, the UB incurred may lurk for years and randomly manifest from time to time and take huge effort to debug.
I don't see how being able to have an & sign on the call site justifies all the weakness pointers inherently carry. moreover, there is std::swap, so c++ programmers are expected and kind of forced to work with lvalue reference out param anyway. I think eigen should at least provide reference overload to open up such usage.. over time, it also provides real data about whether pointer or reference out param is used more often in reality, allowing to make data-driven decisions. or maybe that discussion has been done and the current status quo is already an informed consensus? if so, someone knows eigen better please enlighten. thanks.
I agree. Using pointers as output parameters (as opposed to
references) seems a bit inspired by the Google coding guidelines, but
these have always seemed, uhm, sub-optimal to me.
I find the following much more consistent and obvious:
- const references (or values) designate input parameters,
- const pointers designate purely optional input parameters,
- non-const references designate output parameters, and
- non-const pointers designate purely optional output parameters.
In this scheme, there is no confusion possible; a non-const reference
should never serve as input parameter.
But in most cases, non-optional output parameters should go into the
return type in the first place, and non-const references should be
avoided.
Some may argue that it's harder to see at the call site what a
parameter is, i.e. without looking at the function declaration. In my
opinion this is negligible, but the cost for using pointers as output
variables is incredibly high: the function gets harder to use (have to
add that extra '*', which *should* mean optional!), and much easier to
mis-use (can simply pass nullptr -> boom!).
Best,
Michael
On Mon, Mar 25, 2019 at 5:56 AM Meng Zhu <meng@xxxxxxxxxxxxx> wrote:
>
> trying to understand the justification here. wouldn't pointers as out parameters easily lead to bugs, such as null pointers, or wild pointers? as I read today's [eigen source here](https://github.com/eigenteam/eigen-git-mirror/blob/667b550a1f161ac85bc8f2013939a0adc10af12c/Eigen/src/Core/Visitor.h#L278), `*rowPtr = maxVisitor.row;` looks like a real bug. with references you
> don't get this kind of bugs, so why not?
>
> as for maxCoeff to return coord, does it make sense to use tags to express programmer intention, e.g.,
> auto [value, row, col] = A.maxCoeff(Eigen::return_coord);
>
> basically a form of tag dispatching, but only for overload purpose to live with the original version to cover all usage.
>
> On Fri, Mar 22, 2019 at 9:20 AM David Tellenbach <david.tellenbach@xxxxxxxxxxxxx> wrote:
>>
>> Hello,
>>
>> > We must make sure that this does not introduce overhead if the index(es) is not used. Perhaps with some template magic it is possible to distinguish calling
>> >
>> > auto [value, row,col] = A.maxCoeff();
>> > vs
>> > auto value = A.maxCoeff();
>>
>> I guess it is not possible to realise something like this without (unnecessarily) calculating the indices in the case
>>
>> auto value = A.maxCoeff();
>>
>> If we allow the unnecessary index calculation this is easy to implement. Distinguishing function calls based on return types is usually really hard because return types are not used for template deduction.
>>
>> > If that is not possible, we could of course just give one method a different name.
>>
>> Another alternative would be a static function maxCoeff(A) that returns a struct (or a tuple) but this might introduce more confusion.
>>
>> Cheers,
>> David
>>
>> > On 22. Mar 2019, at 11:54, Christoph Hertzberg <chtz@xxxxxxxxxxxxxxxxxxxxxxxx> wrote:
>> >
>> > We must make sure that this does not introduce overhead if the index(es) is not used. Perhaps with some template magic it is possible to distinguish calling
>> >
>> > auto [value, row,col] = A.maxCoeff();
>> > vs
>> > auto value = A.maxCoeff();
>> >
>> > This would actually be very close to how Matlab "overloads" many functions. If that is not possible, we could of course just give one method a different name.
>> >
>> > Btw: I agree on not overloading for references, as it should be obvious that these are out-parameters. O.t.o.h., with pointers there is always the ambiguity if null-pointers are allowed or not.
>> >
>> > Christoph
>> >
>> > On 22/03/2019 09.45, Gael Guennebaud wrote:
>> >> We chose pointers to emphasise they are out parameters. I would not add
>> >> overloads taking references but maybe a version returning everything within
>> >> a struct to be used with C++17 structure bindings?
>> >> On Thu, Mar 21, 2019 at 3:27 AM Meng Zhu <meng@xxxxxxxxxxxxx> wrote:
>> >>> hi, I noticed eigen maxCoeff function (
>> >>> https://eigen.tuxfamily.org/dox/classEigen_1_1DenseBase.html#a784e23ccbb39e7c57b70af386f94f8b5)
>> >>> takes pointers to return the max entry coord, and there is no overload to
>> >>> take reference type. is there a reason to not provide reference access?
>> >>> thanks.
>> >>>
>> >>> Meng
>> >>>
>> >>>
>> >>>
>> >
>> > --
>> > Dr.-Ing. Christoph Hertzberg
>> >
>> > Besuchsadresse der Nebengeschäftsstelle:
>> > DFKI GmbH
>> > Robotics Innovation Center
>> > Robert-Hooke-Straße 5
>> > 28359 Bremen, Germany
>> >
>> > Postadresse der Hauptgeschäftsstelle Standort Bremen:
>> > DFKI GmbH
>> > Robotics Innovation Center
>> > Robert-Hooke-Straße 1
>> > 28359 Bremen, Germany
>> >
>> > Tel.: +49 421 178 45-4021
>> > Zentrale: +49 421 178 45-0
>> > E-Mail: christoph.hertzberg@xxxxxxx
>> >
>> > Weitere Informationen: http://www.dfki.de/robotik
>> > -------------------------------------------------------------
>> > Deutsches Forschungszentrum für Künstliche Intelligenz GmbH
>> > Trippstadter Strasse 122, D-67663 Kaiserslautern, Germany
>> >
>> > Geschäftsführung:
>> > Prof. Dr. Jana Koehler (Vorsitzende)
>> > Dr. Walter Olthoff
>> >
>> > Vorsitzender des Aufsichtsrats:
>> > Prof. Dr. h.c. Hans A. Aukes
>> > Amtsgericht Kaiserslautern, HRB 2313
>> > -------------------------------------------------------------
>> >
>> >
>> >
>>
>>
>>