Re: [lmi] patch to remove unused argument from numeric_io_cast()

[Greg Chicares]

On 2010-03-23 22:30Z, Vadim Zeitlin wrote:
> 
>  I'd like to ask you what do you think about the following patch:
[drastically abbreviated for clarity in reply]
> --- a/numeric_io_cast.hpp
> 
> -template<typename To, typename From>
> -To numeric_io_cast(From, To = To());
> -
[...]
>  template<typename To, typename From>
> -To numeric_io_cast(From from, To)
> +To numeric_io_cast(From from)
[...]
> ? The very first change is immaterial, I removed the template function
> declaration just because it doesn't seem to be needed, it could be kept if
> you think it's useful (although I'd be curious to know why).

We can prove that it's not a required forward-declaration.

The file dates from 2004, so it wouldn't be some ancient language feature
(like a 'guiding-decl' or whatever).

I don't believe a function template needs a declaration separate from its
definition in order to use a default argument--either in the standard
language or any compiler's dialect (but I could be wrong on this point).

So here's what I think the reason is:
 - The implementation of this function template must follow all the other
     stuff it depends on in this header.
 - I wanted the lengthy '///' block at the top of the header, because
     it explains why that header even exists.
 - I wanted doxygen to link the '///' block with this function template.
     Writing a declaration right after the block accomplishes that.
I know doxygen offers special markup for stuff like that, but I prefer
to avoid it [0] and stick with this simple pattern:

/// Triple-slash comments,
/// in a contiguous block,
/// followed by a blank line, then a declaration.

void TheFunctionOrClassThatIsDocumented()

Simpler is better, but I have another reason: nobody is going to test
whether this stuff works as intended with doxygen, so simpler is also
more robust.

> The really
> important change is the second one which removes the unused "To" argument
> (and the changes to the test just remove the code which doesn't compile
> (and so doesn't need to be tested) any longer).
> 
>  I hope you will accept this patch because the argument is really useless
> here and I think that it's a leftover from the bad old days when a lot of
> compilers didn't support explicitly choosing the specialization of the
> template function to call and so you had to write horrors like
> 
>       numeric_io_cast(from, *static_cast<std::string *>(NULL))
> 
> instead of just
> 
>       numeric_io_cast<std::string>(from)
> 
>  However to the best of my knowledge all the compilers currently used by
> LMI do support the standard syntax and the only compiler which doesn't
> support that I know about is MSVC6 which we definitely don't plan to use.

I could be wrong here, but I suspect that's not the actual reason.
Elsewhere (in 'stream_cast.hpp') we have this:

  template<typename To, typename From>
  To stream_cast(From from, To = To())

which seems to need two arguments to support the function-template
specializations that occur later in that file:

template<> std::string stream_cast<std::string>(char*       from, std::string)
template<> std::string stream_cast<std::string>(char const* from, std::string)
template<> std::string stream_cast<std::string>(std::string from, std::string)

It seems generally preferable to specialize class templates instead:
  http://www.gotw.ca/publications/mill17.htm
but that's not what I did here, and I certainly don't want to rewrite
it now.

Later, numeric_io_cast<>() was written, with the same signature as
stream_cast<>() (probably because there's no reason for them to differ).

>  But the real reason I am submitting this patch is that I just lost more
> than an hour hunting down mysterious crashes and heap corruptions in
> MSVC9-compiled version of LMI. And it turns out that the presence of this
> parameter (or probably its default value) somehow confuses MSVC9 so much
> that it generates completely incorrect code for this function.

Wow. Borland had this right in Philippe Kahn's day.

Let's try to find a less drastic way to avoid this msvc defect. If
  template<typename To, typename From>
  To stream_cast(From from, To = To())
  {...}
is okay, but
  template<typename To, typename From>
  To numeric_io_cast(From, To = To());
is not, then altering something that's similar between the two
just makes them diverge further; but things that are similar ought
to be kept similar to make comprehension and maintenance easier.
And the real problem is likely to be the aspect in which they differ.
Is the forward-declaration the real problem? Can we solve it as follows?

+ #if defined LMI_MSC
  template<typename To, typename From>
  To numeric_io_cast(From, To = To());
+ #endif // defined LMI_MSC

[If so, do you know whether that will break my "robust" doxygen pattern
discussed above?]

---------

[0] "special markup ... but I prefer to avoid it"

David Abrahams said it best:

http://lists.boost.org/Archives/boost/2002/10/38746.php
|
| > @param foo Description of the foo parameter
| > @param bar Description of the bar parameter
| > @return Description of the return value
| > @pre Precondition 1
| > @pre Precondition 2
| > @post Postcondition 1
| > @throws Exception commentary
|
| Yuck!
|
| That should be:
|
|         // foo - description of foo parameter
|         // bar - Description of the bar parameter
|         // Returns: Description of the return value
|         // Preconditions: Precondition 1; Precondition 2
|         // Postcondition: Postcondition 1
|         // Throws: Exception commentary
|
| *Now* you get the idea