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

[Greg Chicares]

On 2014-09-29 14:30Z, Vadim Zeitlin wrote:
> This avoids
> 
>       variable 'name' set but not used [-Werror=unused-but-set-variable]
> 
> warnings/errors generated by at least g++ 4.9.1.
[snip patch]

The other two patches in this series seem obviously correct, while
this one gives me pause. On the one hand, the eliminated variables
plainly seem to be unused. On the other hand, I can't readily see
whether the function calls have any non-obvious purpose: e.g.,
perhaps they have important side effects. For example:

>  renderer_type_convertor const& 
> renderer_type_convertor::get(any_member<Input> const& value)
>  {
> -    mc_enum_base const* as_enum = NULL;
[...]
>      else if(0 != reconstitutor<mc_enum_base  
> ,Input>::reconstitute(nonconst_value))
>          {
> -        as_enum = member_cast<mc_enum_base>(value);
>          return get_impl<renderer_enum_convertor>();
>          }

I'd feel very comfortable moving the declaration and null-initialization
into the else-if block:

-    mc_enum_base const* as_enum = NULL;
...
     else if(0 != reconstitutor<mc_enum_base  
,Input>::reconstitute(nonconst_value))
         {
+        mc_enum_base const* as_enum = NULL;
         as_enum = member_cast<mc_enum_base>(value);
         return get_impl<renderer_enum_convertor>();
         }

, and then combining it with the next line:

-        mc_enum_base const* as_enum = NULL;
-        as_enum = member_cast<mc_enum_base>(value);
+        mc_enum_base const* as_enum = member_cast<mc_enum_base>(value);

, and then even eliminating the variable altogether, e.g.:

-        mc_enum_base const* as_enum = member_cast<mc_enum_base>(value);
+        (void) member_cast<mc_enum_base>(value);

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