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

[Vadim Zeitlin]

On Wed, 24 Mar 2010 13:34:51 +0000 Greg Chicares <address@hidden> wrote:

GC> So here's what I think the reason is:
GC>  - The implementation of this function template must follow all the other
GC>      stuff it depends on in this header.

 Sorry, why? It's not necessary to have the definition of numeric_converter
before the definition of this function, a forward declaration would be
enough. So this patch (against svn) which merges the definition of the
function with its declaration should work just as well:

----------------------------------------------------------------------------
--- a/numeric_io_cast.hpp
+++ b/numeric_io_cast.hpp
@@ -42,6 +42,9 @@
 #   define BOOST_STATIC_ASSERT(deliberately_ignored) class IgNoRe
 #endif // defined __BORLANDC__

+template<typename To, typename From>
+struct numeric_converter;
+
 /// Design notes for function template numeric_io_cast().
 ///
 /// Converts between arithmetic types and their std::string decimal
@@ -104,7 +107,11 @@
 /// integers as numerals.

 template<typename To, typename From>
-To numeric_io_cast(From, To = To());
+To numeric_io_cast(From from, To = To())
+{
+    numeric_converter<To,From> converter;
+    return converter.operator()(from);
+}

 // A compile-time failure iff this template is ever instantiated is
 // desired, but the straightforward
@@ -351,12 +358,5 @@ struct numeric_converter<std::string, char const*>
         }
 };

-template<typename To, typename From>
-To numeric_io_cast(From from, To)
-{
-    numeric_converter<To,From> converter;
-    return converter.operator()(from);
-}
-
 #endif // numeric_io_cast_hpp

----------------------------------------------------------------------------

As before, this was tested with g++ and MSVC9 and the unit tests still pass
for the former.


GC> >  I hope you will accept this patch because the argument is really useless
GC> > here and I think that it's a leftover from the bad old days
<...>
GC> I could be wrong here, but I suspect that's not the actual reason.
GC> Elsewhere (in 'stream_cast.hpp') we have this:
GC> 
GC>   template<typename To, typename From>
GC>   To stream_cast(From from, To = To())
GC> 
GC> which seems to need two arguments to support the function-template
GC> specializations that occur later in that file:
GC> 
GC> template<> std::string stream_cast<std::string>(char*       from, 
std::string)
GC> template<> std::string stream_cast<std::string>(char const* from, 
std::string)
GC> template<> std::string stream_cast<std::string>(std::string from, 
std::string)

 Again, I must be missing something, but I don't see any need for this
parameter. You could specialize it without it just as well, couldn't you?

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

 This might be going OT but I beg to differ. The existence of the second
argument in stream_cast<> is either unnecessary or a workaround for some
technical problem with C++ machinery. It definitely doesn't need it, from
the point of view of the job done by this function, in theory. And exactly
the same applies to numeric_io_cast<>: this function doesn't need a second
parameter, its presence can be only confusing (mostly to people reading the
code but also even to the compilers as I discovered...) and in this case
there are definitely no technical reasons to require it neither. So why
should we be keeping useless parameters like this?

 I was absolutely earnest when I wrote that while I created this patch to
work around an MSVC bug, I hoped you'd accept it from the point of view of
improving code quality alone. I think having unnecessary unused function
parameters is untidy and while I can accept the need for them in some cases
because you can't avoid them, I have trouble understanding why would you
want to keep them in this case.

 To finish my accusatory speech against this unneeded parameter let me
say that having both "From" and "To" arguments in this function definitely
did make understanding the code more difficult to me (especially because
their order is reversed compared to the order of template parameters) and
that having just one argument would have undoubtedly saved me some time by
allowing me to see what was going on faster.



GC> Let's try to find a less drastic way to avoid this msvc defect. If
GC>   template<typename To, typename From>
GC>   To stream_cast(From from, To = To())
GC>   {...}
GC> is okay, but
GC>   template<typename To, typename From>
GC>   To numeric_io_cast(From, To = To());
GC> is not, then altering something that's similar between the two
GC> just makes them diverge further; but things that are similar ought
GC> to be kept similar to make comprehension and maintenance easier.

 The patch above does make them more similar and I think it's preferable to
using an explicit "#if".

GC> And the real problem is likely to be the aspect in which they differ.
GC> Is the forward-declaration the real problem? Can we solve it as follows?
GC> 
GC> + #if defined LMI_MSC

 This was supposed to be "#if !defined LMI_MSC", right?

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

 I didn't test this but I'm pretty sure that this would solve it too as the
preprocessed code would be exactly the same as I tested. However



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

 No, Doxygen includes a preprocessor and unless you specifically tell it to
consider LMI_MSC defined, it will ignore "#ifndef LMI_MSC" lines, i.e. will
work as if they were not there at all.

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

 This would take even further OT again but IME you can't avoid it. You can
have either readable source code (LMI) or readable documentation (any
typical project using Doxygen) or both at the cost of duplication (wx keeps
its headers readable by putting Doxygen documentation in another set of
headers). I didn't try running Doxygen on LMI sources (I could try it if
you're curious...) but I'm pretty sure that you're not going to get
anything nice out of it.

 Regards,
VZ