[PATCH] Ensure std::getline always 0-terminates string.

[Volodymyr Sapsai via cfe-commits]

On Nov 12, 2017, at 12:37, Reimar Döffinger <Reimar.Doeffinger at gmx.de> wrote:
> 
> On Thu, Nov 09, 2017 at 05:37:32PM -0800, Volodymyr Sapsai wrote:
>> On Nov 9, 2017, at 12:13, Reimar Döffinger <Reimar.Doeffinger at gmx.de> wrote:
>>> 
>>> Hello!
>>> 
>>> On Wed, Nov 08, 2017 at 12:36:00PM -0800, Volodymyr Sapsai wrote:
>>>> Thanks for the patch, Reimar. Can you please add tests to ensure this functionality doesn’t regress? As null character is required by the standard (27.7.2.3p21), a good starting point seems to be
>>>> test/std/input.output/iostream.format/input.streams/istream.unformatted/getline_pointer_size.pass.cpp
>>>> test/std/input.output/iostream.format/input.streams/istream.unformatted/getline_pointer_size_chart.pass.cpp
>>> 
>>> New patch attached, though I went the lazy way of just adding one case.
>>> Not sure that's "good enough" - in principle I think it should be.
>> I think it makes sense to cover wchar_t as well. And I think it would be useful to test the case with specified delimiter too. Most likely implementation should be reused for the case with explicit delimiter and default new line delimiter. But I prefer not to rely on this assumption and test explicitly.
> 
> Well, it was rather trivial to do anyway, so done.
Thanks for adding more tests.

>>> More tricky would be to add a test for the _LIBCPP_NO_EXCEPTIONS case,
>>> is there any code testing that at all?
>> According to tests, exceptions are tested in various ways. Let me find how to configure the build to run tests in these modes.
> 
> There aren't many references to that mode, and I am not sure
> running in a separate mode would make for a good test anyway.
> Maybe not how it should done, but for now I added a separate file
> that just defines _LIBCPP_NO_EXCEPTIONS before the includes.
> <0001-Ensure-std-istream-getline-always-0-terminates-strin.patch>

libc++ can be built with exceptions enabled or disabled (see LIBCXX_ENABLE_EXCEPTIONS <http://libcxx.llvm.org/docs/BuildingLibcxx.html#cmdoption-arg-libcxx-enable-exceptions>) and we need to support both configurations. The problem with your _LIBCPP_NO_EXCEPTIONS approach is that it won't work when exceptions are disabled. Various exception-related tests are usually guarded with TEST_HAS_NO_EXCEPTIONS macro defined in “test_macros.h”. You can check other tests for examples of TEST_HAS_NO_EXCEPTIONS usage, I had in mind something like

#ifndef TEST_HAS_NO_EXCEPTIONS
    {
        testbuf<char> sb(" ");
        std::istream is(&sb);
        char s[5] = "test";
        is.exceptions(std::istream::eofbit | std::istream::badbit);
        try
        {
            is.getline(s, 5);
            assert(false);
        }
        catch (std::ios_base::failure&)
        {
        }
        assert( is.eof());
        assert( is.fail());
        assert(std::string(s) == " ");
        assert(is.gcount() == 1);
    }
#endif

Non-empty `sb` allows to test null-termination at correct place in array; initializing `s` with “test” avoids spurious correct values in memory; try-catch block and badbit are to make it more obvious we are testing code path throwing an exception.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20171115/7f742459/attachment.html>