Re: [lmi] [PATCH] Fix trailing whitespace detection in stream_cast()

[Greg Chicares]

On 2020-01-27 17:13, Vadim Zeitlin wrote:
> 
>  I'd like to return to the issue first discussed in this message:
> 
>       https://lists.nongnu.org/archive/html/lmi/2018-11/msg00034.html
> 
> This might look like a minor problem, but it's really annoying to have to
> juggle with the additional patch when testing lmi either with clang or
> MSVC, so I'd like to propose the patch at
>  
>       https://github.com/vadz/lmi/pull/136
> 
> for inclusion in lmi master.
> 
>  This patch simply exchanges the order of ws() and eof() checks

The first thing I wonder about is how this became wrong. I thought the
idiom was ancient and generally accepted--cf. this from late 2000:

http://www.two-sdg.demon.co.uk/curbralan/code/lexical_cast/lexical_cast.hpp

| template<typename Target, typename Source>
| Target lexical_cast(Source arg)
| {
|     std::strstream interpreter; // for out-of-the-box g++
|     Target result;
| 
|     if(!(interpreter << arg) || !(interpreter >> result) ||
|        !(interpreter >> std::ws).eof())
|         throw bad_lexical_cast();
|
|     return result;
| }

and I figured that any C++ standard change that breaks it would get
reverted if we just wait.

But now I wonder whether I introduced the error, locally, when I split
    !(interpreter >> std::ws).eof())
into
    else if(!(interpreter >> std::ws))
        {
        err << "Trailing whitespace remains ";
        }
    else if(!interpreter.eof())
        {
        err << "Unconverted data remains ";
        }
?

The problem is that
  std::istringstream interpreter("123");
  int n = -1;
  interpreter >> n;
leaves the stream at EOF, and subsequently, therefore,
  interpreter >> std::ws
is supposed to set failbit, so the standard requires that this code:

    else if(!(interpreter >> std::ws))
        {
        err << "Trailing whitespace remains ";
        }

write the "Trailing whitespace remains " message into 'err'. Right?

But, given the same initial steps:
  std::istringstream interpreter("123");
  int n = -1;
  interpreter >> n;
what is this code
  !(interpreter >> std::ws).eof())
supposed to do?
 - it invokes the std::ws extractor; in this case, eofbit was set
by the ctor, so the effect is to set failbit; then
 - eof() is called; it ignores failbit, inspects eofbit, and
returns true. Right?

Do you concur with my conclusion that I introduced this defect by
splitting the
  !(interpreter >> std::ws).eof())
line incorrectly--in particular, by invoking
  std::ios_base::operator!()
in my version, when Henney's original did not invoke operator!()
(even though it would leave the stream flags in the same state)?

>, which is
> sufficient for working around libstdc++ bug which results in the currently
> clearly wrong code (which checks that there is _no_ whitespace left in the
> string instead of testing that some whitespace is left) still working when
> using this standard library implementation.
> 
>  The advantages of this patch are:
> 
> - It makes things work (i.e. the new unit tests pass, but, arguably even
>   more importantly, lmi can now construct Input objects successfully) when
>   using clang/MSVC.
> 
> - It fixes the "!(interpreter >> std::ws)" check which just doesn't make
>   any sense when reading the code.
> 
> - It improves the error message in case there are actually any trailing
>   spaces, as it was clearly supposed to be "Trailing whitespace remains" 
>   and not the more generic "Unconverted data remains".

Okay, perhaps this is what I should have done when I purposed to
distinguish those two error conditions.

BTW, why add the 'calendar_date' TU to this unit test? Is that only
because your original report arose on this line:

input.cpp:      ,ListBillDate                     {"2440588"} // Assume no 
inforce so old

  https://lists.nongnu.org/archive/html/lmi/2018-11/msg00034.html
|  I've been looking at stream_cast() code because it started throwing
| "Trailing whitespace remains" exception with the latest MSVS version
| (15.9.0) when initializing Input::ListBillDate field from its JDN given in
| the string form in this class ctor

I see no calendar_date-specific overload of stream_cast, so I'm
guessing there's no deeper reason; therefore, I'd probably prefer
to replace this with a plain string conversion in order to keep
the unit test minimal and self-contained.