On May 28, 2014, at 10:27 PM, David Majnemer <[email protected]> wrote:
> Updated to address review comments. LGTM - thanks! — Marshall > > > On Wed, May 28, 2014 at 7:23 AM, Marshall Clow <[email protected]> wrote: > > On May 27, 2014, at 11:44 AM, David Majnemer <[email protected]> wrote: >> On Tue, May 27, 2014 at 7:45 AM, Marshall Clow <[email protected]> wrote: >> >> On May 20, 2014, at 2:25 AM, David Majnemer <[email protected]> wrote: >> >>> Oops, sent out the wrong version of this patch. Attached is what I >>> intended to send. >>> >>> >>> On Tue, May 20, 2014 at 12:28 AM, David Majnemer <[email protected]> >>> wrote: >>> random_device::operator() as currently implemented does not correctly >>> handle errors returned by read. This can result in it returning >>> uninitialized data. >>> >>> To fix this, wrap the call to read in a loop. >> >> I like this; but can you think of any way to test it? >> >> I've added a test for the EOF case. >> >> I have written a test for EINTR case but I did not included it because it's >> inherently not reliable. It checks the output of operator() to see if >> signals resulted in us getting a lot of zero return results. > > I get a couple of signedness warnings when building: > > + for FILE in '../src/*.cpp' > + /Sources/LLVM/bin/bin/clang++ -c -g -Os -arch i386 -arch x86_64 -nostdinc++ > -std=c++11 -fstrict-aliasing -Wall -Wextra -Wshadow -Wconversion -Wpadded > -Wstrict-aliasing=2 -Wstrict-overflow=4 -I../include ../src/random.cpp > ../src/random.cpp:78:14: warning: implicit conversion changes signedness: > 'ssize_t' (aka 'long') to 'unsigned long' [-Wsign-conversion] > i += s; > ~~ ^ > 1 warning generated. > ../src/random.cpp:78:14: warning: implicit conversion changes signedness: > 'ssize_t' (aka 'long') to 'unsigned long' [-Wsign-conversion] > i += s; > ~~ ^ > 1 warning generated. > > I get that read can only return -1, 0, and positive numbers, and you’re > already checking for -1 and 0, so I think that you can safely cast s to a > size_t before doing the addition. > > > Also, from a readability standpoint, wouldn’t a while loop be better than the > for loop? > Something like this: > > while (n > 0) > { > ssize_t s = read(__f_, p, n); > if (s == 0) > __throw_system_error(ENODATA, "random_device got EOF"); > if (s == -1) > { > if (errno != EINTR) > __throw_system_error(errno, "random_device got an unexpected > error"); > continue; > } > i -= (size_t) s; > p += (size_t) s; > } > > > > <t.patch>
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
