NetBSD alread defines bswap16

[GoogleCodeExporter]

On NetBSD, including stdlib.h pulls in bswap.h which defines bswap16.

Reference error: 
http://www.cpantesters.org/cpan/report/34d82f1a-71a5-11e1-8755-e63c2a028b78

You might be able to borrow some of the portability tweaks from csnappy: 
https://github.com/zeevt/csnappy/blob/master/csnappy_internal_userspace.h

Original issue reported on code.google.com by g...@cpan.org on 19 Mar 2012 at 6:09

[GoogleCodeExporter]

Thanks for the hint. I will look into it.

Original comment by yann.col...@gmail.com on 19 Mar 2012 at 6:17

• Changed state: Accepted

[GoogleCodeExporter]

There are 2 ways to look at this issue.

If i do understand correctly, the error is caused by an unfortunate duplicate 
of bswap16() function.
This could be solved by providing another name to the function, such as 
lz4_bswap16() for instance. This solution would create the less modifications.

On the other hand, i see 2 good reasons to consider a larger change inspired by 
the suggested source code :
- First, i had one report from Erik Andersen that, sometimes, bswap16() seems 
not translated into intrinsic (hardware) opcode. This is surprising, as GCC 
comment was very clear that such a sequence should be detected and optimized on 
the fly by the compiler. Compared to hardware swap, un-optimzed software swap 
causes a small slow down of a few %. 
It seems csnappy_internal_userspace.h would solve this issue for a fair number 
of OS.

- Second : i had 2 reports of failed endian detection (mostly on ARM systems). 
See for example : 
https://groups.google.com/forum/?fromgroups#!topic/lz4c/ZBU8r0-tPaE . 
Specifically, it seems the pre-compiled macro is tricked into believing that it 
will be compiled for Big-endian, while in fact it is going to be running on a 
little-endian environment (ARM are bi-endians).
csnappy_internal_userspace.h seems also to address this issue.

The second issue seems more important.

There is a downside however, in that the added code is relatively large, 
bloating the source a bit more. It is also specific to a (pretty respectable) 
set of OS. So it's unclear to me what would happen with an unlisted OS. I guess 
it would probably fail.

Original comment by yann.col...@gmail.com on 20 Mar 2012 at 10:25

[GoogleCodeExporter]

The following attached file is a candidate for r61, which changes 2 elements :

- The interface contracts evolves, to use "size_t" type instead of "int" for 
all memory distances. This is required for the (very rare) case of extremely 
large memory buffers going beyond "int" limits (2GB).
I'm supportive of the change because it seems to not affect existing code, 
being completely transparent. In my tests, no modification was necessary in any 
user program.

Now, what's true for C programs might not be for language binders, such as 
Perl. Would you mind telling me if the change is also transparent for you ?

- I've used this opportunity to also "quick fix" issue 15, by changing the name 
of the macro to lz4_bswap16(). It should avoid any duplicate, such as the 
NetBSD one.

I'm still interested in the "snappy-like" detection macros, but want more time 
to test it.

Original comment by yann.col...@gmail.com on 23 Mar 2012 at 8:40

Attachments:

• LZ4rc61.zip

[GoogleCodeExporter]

Works for the Perl binding, except that LZ4_uncompress now returns a size_t, 
even though it's supposed to be able to return a negative number on error.

Original comment by g...@cpan.org on 23 Mar 2012 at 5:07

[GoogleCodeExporter]

uh oh, excellent point, i need to figure out a way to properly handle error 
codes before releasing r61...

Thanks for feedback Gray

Original comment by yann.col...@gmail.com on 27 Mar 2012 at 3:19

[GoogleCodeExporter]

Finally implemented the quick fix (macro name changed to avoid duplicate) 
within r61. Interface remains unchanged, using "int" type for memory sizes.

Original comment by yann.col...@gmail.com on 3 Apr 2012 at 7:37

• Changed state: Fixed

[GoogleCodeExporter]

The macro also needs to be renamed in lz4hc.c.

Original comment by gray...@gmail.com on 11 Aug 2012 at 3:50

[GoogleCodeExporter]

Thanks for notification Gray. I'll look into it.

Original comment by yann.col...@gmail.com on 13 Aug 2012 at 7:34

[GoogleCodeExporter]

Hi Gray

The following source package (candidate r76)
should solve the reported issue.
Would you mind telling if it works for you ?

Best Regards

Original comment by yann.col...@gmail.com on 13 Aug 2012 at 11:45

Attachments:

• lz4_rc76.zip

[GoogleCodeExporter]

I had already made the macro change in my copy before commenting. And there's a 
PASS entry for NetBSD here: 
http://static.cpantesters.org/distro/C/Compress-LZ4.html#0.15 , so that's 
confirmation of the resolution. The extra casting changes in r76 shouldn't 
affect that.

Original comment by gray...@gmail.com on 13 Aug 2012 at 4:36

[GoogleCodeExporter]

OK Thanks.
Btw, this PASS tool looks excellent to test the code on a myriad of 
configurations

Original comment by yann.col...@gmail.com on 14 Aug 2012 at 4:26