Issue 29481704: Noissue - Use buffer for FileSystem IO

Issue 29481704: Noissue - Use buffer for FileSystem IO (Closed)

Created:
July 6, 2017, 9:19 p.m. by hub

Modified:
July 7, 2017, 1:19 p.m.

Reviewers:
sergei

CC:
Felix Dahlke

Base URL:
https://hg.adblockplus.org/libadblockplus/

Visibility:
Public.

Patch Set 2 : Removed Utils::Slurp() we no longer need #

Patch Set 3 : Nit addressed #

Created: July 7, 2017, 12:50 p.m.

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+57 lines, -63 lines)			Patch
M	include/AdblockPlus/DefaultFileSystem.h	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	include/AdblockPlus/FileSystem.h	View	1 2	1 chunk	+6 lines, -5 lines	0 comments	Download
M	src/DefaultFileSystem.cpp	View		1 chunk	+15 lines, -6 lines	0 comments	Download
M	src/FileSystemJsObject.cpp	View		2 chunks	+4 lines, -5 lines	0 comments	Download
M	src/Utils.h	View	1	1 chunk	+0 lines, -2 lines	0 comments	Download
M	src/Utils.cpp	View	1	1 chunk	+0 lines, -8 lines	0 comments	Download
M	test/BaseJsTest.h	View		2 chunks	+5 lines, -5 lines	0 comments	Download
M	test/DefaultFileSystem.cpp	View		1 chunk	+4 lines, -6 lines	0 comments	Download
M	test/FileSystemJsObject.cpp	View		3 chunks	+10 lines, -13 lines	0 comments	Download
M	test/Prefs.cpp	View		2 chunks	+11 lines, -11 lines	0 comments	Download

Messages

Total messages: 4

Expand All Messages | Collapse All Messages

sergei

LGTM https://codereview.adblockplus.org/29481704/diff/29481705/include/AdblockPlus/FileSystem.h File include/AdblockPlus/FileSystem.h (right): https://codereview.adblockplus.org/29481704/diff/29481705/include/AdblockPlus/FileSystem.h#newcode78 include/AdblockPlus/FileSystem.h:78: Read(const std::string& path) const = 0; I think ...

July 7, 2017, 7:27 a.m. (2017-07-07 07:27:36 UTC) #2

hub

just addressed the nit. https://codereview.adblockplus.org/29481704/diff/29481705/include/AdblockPlus/FileSystem.h File include/AdblockPlus/FileSystem.h (right): https://codereview.adblockplus.org/29481704/diff/29481705/include/AdblockPlus/FileSystem.h#newcode78 include/AdblockPlus/FileSystem.h:78: Read(const std::string& path) const = ...

July 7, 2017, 12:51 p.m. (2017-07-07 12:51:00 UTC) #3

sergei

July 7, 2017, 1:19 p.m. (2017-07-07 13:19:37 UTC) #4

Message was sent while issue was closed.

LGTM

https://codereview.adblockplus.org/29481704/diff/29481705/src/DefaultFileSyst...
File src/DefaultFileSystem.cpp (right):

https://codereview.adblockplus.org/29481704/diff/29481705/src/DefaultFileSyst...
src/DefaultFileSystem.cpp:81:
file.read(reinterpret_cast<std::ifstream::char_type*>(data.data()),
On 2017/07/07 12:51:00, hub wrote:
> On 2017/07/07 07:27:36, sergei wrote:
> > I would prefer to rather use static_cast but it seems fine here.
> 
> static_cast<> is rejected by the compiler:
> 
> ../src/DefaultFileSystem.cpp:81:13: error: static_cast from 'value_type *'
(aka
> 'unsigned char *') to 'std::ifstream::char_type *' (aka 'char *') is not
allowed
>   file.read(static_cast<std::ifstream::char_type*>(data.data()),
>             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 1 error generated.
> 

Yeah, one has to firstly cast it to void*, basically reinterpret_cast is fine.

Expand All Messages | Collapse All Messages