Re: [lmi] more questions due to MSVC warnings

[Greg Chicares]

On 2008-03-24 18:21Z, Vadim Zeitlin wrote:
> 
>  There is another code which results in MSVC warnings ("local variable
> may be used without having been initialized" this time) that I don't really
> know how to fix but which might indicate a problem. This is in
> interest_rates.cpp, in the function called convert_interest_rates(). There
> the cached_{monthly,annual}_net_rate variables may not be set during the
> first loop iteration) or changed (during the subsequent ones) if the
> condition in the if at lines 215..219 is false. But they're assigned to the
> elements of {monthly,annual}_net_rate in any case. Again, I don't know if
> this is a real problem or if the condition is always true or if it doesn't
> matter what we assign to these arrays in the case it is false but it would
> seem that these assignments should be inside the if statement, not outside
> it. What do you think?

I wish I'd written unit tests for this class. Then we could try
speculative changes and find out in a few seconds whether they're
valid or not.

I guess the condition is always true on the first loop iteration,
in any "plausible" case. But even if I felt sure about such an
unasserted and undocumented invariant, I'd want to fix this code
so that it would handle "impossible" cases appropriately.

The assignments definitely do belong outside the conditional:
  if cached-value would be invalid
    then recompute cached-value
    end-if
  assign real-value from cached-value

Probably the correct fix would be to zero-initialize the two
uninitialized local variables. I'd need to think that through
carefully and run all regression tests (some of which use
proprietary datasets that you don't have).

I would gladly accept a patch such as this in the meantime:

+     // TODO ?? Rectify this apparent logic error. See:
+     //   [insert this message's URL]
+ #if !defined __SOME_MSVC_MACRO__
      double cached_annual_net_rate;
      double cached_monthly_net_rate;
+ #else  // defined __SOME_MSVC_MACRO__
+     double cached_annual_net_rate     = 0.0;
+     double cached_monthly_net_rate    = 0.0;
+ #endif // defined __SOME_MSVC_MACRO__

and then clean it up when time permits.

>  I'd also like to know how to deal with the same warning when it definitely
> does _not_ indicate a problem, like in stream_cast() function in the header
> with the same name. There the "result" variable will only be returned if it
> was successfully initialized (by reading it from the stream) but the
> compiler doesn't realize this and still warns about it being potentially
> uninitialized. I think it would be worth to add " = To()" to the variable

I agree. That seems to be the most tasteful solution. It could be
less efficient if default-constructing a 'To' object is costly,
but we already perform that operation for the default argument
(which, IIRC, was added as a workaround for some compiler), so
we've already decided not to care about that; furthermore, any
decent compiler would still call the default ctor exactly once,
AIUI, with or without your suggested change. Or zero times for a
POD object; but I'm spending too much time thinking about this.

There are probably some places in ancient code where I left a
local variable uninitialized, precisely in order to avoid a bogus
borland warning about an unused initial value; now I would call
that a bad workaround for a warning that would better have been
suppressed. At the moment I can't think of a case where it would
be preferable not to initialize a local at point of declaration.

BTW, this function is an example of compiler conditionals gone
wild. I ought to have worked through the libcomo issues with
comeau. And I should have abandoned the borland toolchain long
ago; I'm just reluctant to erase debugged code now.

> declaration just to suppress this warning as I'd like to keep it enabled
> globally because it can help to discover real bugs sometimes (whether the
> problem in interest_rates.cpp is real or not). Would you agree with this?

I do agree.