sonjeheon / lz4

Automatically exported from code.google.com/p/lz4
0 stars 0 forks source link

lz4 doesn't declare source arrays as const #6

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
I'm attaching a patch which properly declares source arrays as const.

This is needed to prevent warnings or errors when passing const source data to 
LZ4 methods.

James

Original issue reported on code.google.com by caprifin...@gmail.com on 30 Jan 2012 at 8:11

Attachments:

GoogleCodeExporter commented 9 years ago
Thanks for your comment.
Indeed, i've been pondering this decision for quite some time now.

The main con is that it would change the interface contract with existing code, 
creating "warnings" for them. So i'm wondering which decision is best.

I'll take your vote for a "const source" into consideration.

Original comment by yann.col...@gmail.com on 30 Jan 2012 at 9:57

GoogleCodeExporter commented 9 years ago
When it's non-const, it makes it very difficult to integrate with existing code 
because the compression pathways in existing applications tend to assume that 
the input to a compressor will always be const.  Indeed, all of the "fast" 
compression APIs I've worked with (LZO, Snappy, QuickLZ) take the source char 
array as const.  So forcing non-const makes it more difficult to use LZ4 as a 
drop-in replacement for other algs.

There is also the problem that the non-const declaration forces the developer 
to reverse engineer the code to see if LZ4 is actually modifying the source 
arrays.  Because if source is being modified, it changes the semantics, from 
the client perspective.  Now, since I don't know whether or not source is being 
modified, I have to make a copy of it before I pass it to LZ4, since LZ4 is not 
offering a guarantee of read-only semantics on source.

I don't think there will be any issues because casting from non-const to const 
is straightforward and causes no warnings on either C or C++ (unlike the 
reverse).  I would think that any code that relies on the current API signature 
would be fine with such a change.

James

Original comment by caprifin...@gmail.com on 30 Jan 2012 at 8:41

GoogleCodeExporter commented 9 years ago
One should not over-value the protection offered by "const".
It is in fact merely an "hint". Nothing prevents the function from modifying 
the source, even if provided through a "const char*" pointer. We just have to 
'trust' it.
If you don"t trust a function you may provide it with a read-only memory area, 
or scrutinize the source code.

Regarding the impact on existing code :
I have notice one situation where const/normal makes a difference : function 
pointers. In this case, the compiler does not agree "it is the same".
Apart from this case, it seems older code does nicely accept the change.

Since i agree that it makes sense to declare source as "const", and since 
impact on existing code is small, i'll make the change.

Original comment by yann.col...@gmail.com on 1 Feb 2012 at 4:48

GoogleCodeExporter commented 9 years ago
I'll handle this one.

Original comment by yann.col...@gmail.com on 1 Feb 2012 at 4:51

GoogleCodeExporter commented 9 years ago
interface modified in r53

Original comment by yann.col...@gmail.com on 1 Feb 2012 at 8:35