Re: [lmi] [PATCH 2/3] Remove unused variables.

[Vadim Zeitlin]

On Wed, 01 Oct 2014 13:15:47 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2014-09-29 14:30Z, Vadim Zeitlin wrote:
GC> > This avoids
GC> > 
GC> >   variable 'name' set but not used [-Werror=unused-but-set-variable]
GC> > 
GC> > warnings/errors generated by at least g++ 4.9.1.
GC> [snip patch]
GC> 
GC> The other two patches in this series seem obviously correct, while
GC> this one gives me pause.

 Sorry, I should have been more explicit about it. I was, of course, also
surprised by the presence of these unused variables, and it took time to
convince myself that they were really unused. However after examining the
code and its history carefully, it does seem like it is the case.

GC> I'd feel very comfortable moving the declaration and null-initialization
GC> into the else-if block:
GC> 
GC> -    mc_enum_base const* as_enum = NULL;
GC> ...
GC>      else if(0 != reconstitutor<mc_enum_base  
,Input>::reconstitute(nonconst_value))
GC>          {
GC> +        mc_enum_base const* as_enum = NULL;
GC>          as_enum = member_cast<mc_enum_base>(value);
GC>          return get_impl<renderer_enum_convertor>();
GC>          }
GC> 
GC> , and then combining it with the next line:
GC> 
GC> -        mc_enum_base const* as_enum = NULL;
GC> -        as_enum = member_cast<mc_enum_base>(value);
GC> +        mc_enum_base const* as_enum = member_cast<mc_enum_base>(value);
GC> 
GC> , and then even eliminating the variable altogether, e.g.:
GC> 
GC> -        mc_enum_base const* as_enum = member_cast<mc_enum_base>(value);
GC> +        (void) member_cast<mc_enum_base>(value);

 FWIW I can confirm that this does get rid of the warning, so it's an
acceptable solution from my point of view.

GC> Making sure that the function call can safely be eliminated requires
GC> more thought. Here, 'value' is an argument passed by const reference,
GC> so member_cast<>() can't change it. The only side effect I can see is
GC> that this function template throws an exception if 'value' is invalid:
GC> thus, calling it without actually using its return value might have
GC> been an intentional means of "implicit" error checking; it's hard to
GC> prove that there was no such intention. You know Vaclav's style better
GC> than I do; what do you think he had in mind when he wrote this?

 Vaclav has already confirmed that he didn't do it intentionally (thanks!),
but I did analyze exactly this possibility (of throwing an exception) and
convinced myself that it didn't really matter. Unfortunately I didn't think
to write down my reasoning, so I'm not sure about it any more, but I could
retrace my steps if you think it would be useful.

 Please let me know if you'd like me to do it, thanks,
VZ