[vtk-developers] Request for a gentler tone in commit message verbiage...

[David Cole]

Thanks for the discussion.

I did try what you said, and I do reproduce the error you saw, ... but
I still say it's not a general problem with double and make_pair.

make_pair ought to be use-able without specifying types. If you have
to specify the type of a pair, then you should just use pair<type1,
type2> directly as you've ended up with. The extra cast you needed for
VS2010 is the real shame.

There would be 4 alternative ways to write the same line of code that
do use make_pair. All of these work with VS 2013, and I suspect they
would also work with VS 2010. (I'll verify if you would like me to.)

    vtkTextProperty *prop = NULL;
    std::make_pair(value, prop);
    std::make_pair(value, reinterpret_cast<vtkTextProperty*>(NULL));
    std::make_pair(value, static_cast<vtkTextProperty*>(NULL));
    std::make_pair(value, (vtkTextProperty*) NULL);

More discussion of the general issue:
http://stackoverflow.com/questions/9270563/purpose-of-stdmake-pair

I will propose a patch that rephrases the in-code comments about this
if nobody else has the time to do so.


Thanks,
David C.


On Wed, Apr 29, 2015 at 4:04 PM, Robert Maynard
<robert.maynard at kitware.com> wrote:
> It has to be 2012 or newer as this is a noted break in functionality
> between C++98 and C++11 ( and forward ).
>
> Here is the excerpt on what has changed:
> "
>
> Following a breaking change between the C++98/03 and C++11 standards,
> using explicit template arguments to call make_pair()—as in
> make_pair<int, int>(x, y)—typically does not compile in Visual C++ in
> Visual Studio 2012. The solution is to always call make_pair() without
> explicit template arguments—as in make_pair(x, y). Providing explicit
> template arguments defeats the purpose of the function. If you require
> precise control over the resulting type, use pair instead
> of make_pair—as in pair<short, short>(int1, int2).
>
>
> Another breaking change between the C++98/03 and C++11 standards: When
> A is implicitly convertible to B and B is implicitly convertible to C,
> but A is not implicitly convertible to C, C++98/03 and Visual C++ 2010
> permitted pair<A, X> to be converted (implicitly or explicitly) to
> pair<C, X>. (The other type, X, is not of interest here, and this is
> not specific to the first type in the pair.) Because C++11 and Visual
> C++ in Visual Studio 2012 detect that A is not implicitly convertible
> to C, they remove the pair conversion from overload resolution. This
> is a positive change for many scenarios. For example, overloading
> func(const pair<int, int>&) and func(const pair<string, string>&), and
> calling func() with pair<const char *, const char *> will compile with
> this change. However, this change breaks code that relied on
> aggressive pair conversions. Such code can typically be fixed by
> performing one part of the conversion explicitly—for example, by
> passing make_pair(static_cast<B>(a), x) to a function that expects
> pair<C, X>.
>
> "
> The reason that we haven't seen this with other compilers is either
> they haven't enabled C++11 (with c++11 standard library) or they
> compile options are lax and allow this behavior. This is the challenge
> of supporting Visual Studio 2010 and forward, they only compile with
> C++11 enabled which causes these very subtle issues.
>
> Note because we require C++98 support we can't use the approach that
> Visual Studio recommends
>
> article: https://msdn.microsoft.com/en-us/library/vstudio/bb531344%28v=vs.110%29.aspx
>
> On Wed, Apr 29, 2015 at 3:54 PM, David Lonie <david.lonie at kitware.com> wrote:
>> On Wed, Apr 29, 2015 at 3:44 PM, Sean McBride <sean at rogue-research.com>
>> wrote:
>>>
>>> On Wed, 29 Apr 2015 13:50:53 -0400, David Cole via vtk-developers said:
>>>
>>> >claims there was some sort of problem with make_pair and double. What
>>> >exactly was the problem? With what exact version of MSVC? (I don't buy
>>> >that there's a general problem with make_pair and double...)
>>>
>>> I'll steer clear of the larger point... :)  but will just say: when adding
>>> workarounds for problematic compilers / OSes, it would be very appreciated
>>> to be clear about which versions.  As someone who has gone through VTK to
>>> expunge obsolete workarounds, it's sometimes been hard to know what's
>>> obsolete and what's not.  A lot of workaround notes say things like "on
>>> Apple this is broken", but which OS version is not stated...
>>>
>>> Try to think of the future in commit messages and code comments.  :)
>>
>>
>> +1. In this case, the workaround is still valid code, no alternate paths,
>> and just a slight change in syntax, so we won't have to worry about removing
>> it in the future :D I did leave a note in-line warning future maintainers
>> that the line is troublesome for MSVC and should be treated with caution.
>>
>> I am somewhat curious what version of the compiler this was too, I can't
>> really tell from the CDash page:
>>
>> https://open.cdash.org/viewBuildError.php?buildid=3784595
>>
>> IIRC, we used to have this prominently displayed in the build name. Looks
>> like that got dropped somewhere along the way. I'll see if we can get that
>> info back in.
>>
>> Dave
>>
>>
>> _______________________________________________
>> Powered by www.kitware.com
>>
>> Visit other Kitware open-source projects at
>> http://www.kitware.com/opensource/opensource.html
>>
>> Search the list archives at: http://markmail.org/search/?q=vtk-developers
>>
>> Follow this link to subscribe/unsubscribe:
>> http://public.kitware.com/mailman/listinfo/vtk-developers
>>
>>
> _______________________________________________
> Powered by www.kitware.com
>
> Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html
>
> Search the list archives at: http://markmail.org/search/?q=vtk-developers
>
> Follow this link to subscribe/unsubscribe:
> http://public.kitware.com/mailman/listinfo/vtk-developers
>