VC2005 link time error (DEBUG VERSION) about lz4

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

here is the patch:

//****************************
// Private functions
//****************************
#if LZ4_ARCH64

#if defined(_MSC_VER) && !defined(LZ4_FORCE_SW_BITCOUNT)
#   if defined(LZ4_BIG_ENDIAN)
#       pragma intrinsic(_BitScanReverse64)
#   else
#       pragma intrinsic(_BitScanForward64)
#   endif
#endif

// ...

#else

#if defined(_MSC_VER) && !defined(LZ4_FORCE_SW_BITCOUNT)
#   if defined(LZ4_BIG_ENDIAN)
#       pragma intrinsic(_BitScanReverse)
#   else
#       pragma intrinsic(_BitScanForward)
#   endif
#endif

Original comment by [email protected] on 12 Apr 2012 at 9:53

[GoogleCodeExporter]

OK, I will look into it. Although i haven't VC2005 available around...

Original comment by [email protected] on 12 Apr 2012 at 7:05

• Changed state: Accepted

[GoogleCodeExporter]

I believe this issue not only appeared in version 8 (2005). 

Original comment by [email protected] on 12 Apr 2012 at 7:09

[GoogleCodeExporter]

Please find in attached file a proposed correction.
Would you mind checking if it works for you ?

Note that the correction only relates to issue 16 (VS2005 linking error on 
Debug).

Regards

Original comment by [email protected] on 12 Apr 2012 at 7:16

Attachments:

• lz4.zip

[GoogleCodeExporter]

You have only fixed it for 32bit platform?

Original comment by [email protected] on 13 Apr 2012 at 7:47

[GoogleCodeExporter]

> I believe this issue not only appeared in version 8 (2005). 

Apparently, the issue is not present within Visual 2008 nor 2010 (I could test 
these versions). I guess it is specific to Visual 2005 and maybe earlier 
versions.


> You have only fixed it for 32bit platform?

Yes.

I am aware of no situation using VS2005 to build 64 bits binaries.
I understand that in theory it could happen, but i also deeply believe that it 
is detrimental to bloat the generic code too much, so there is a trade-off to 
select. Covering too specific corner cases should be left to forks.

Even the current correction seems a bit too focused, since it does not target 
the Microsoft family of compilers, but only the specific 2005 version. But it's 
okay because it is only 2 lines and produces no extra-case.

Original comment by [email protected] on 13 Apr 2012 at 11:20

[GoogleCodeExporter]

I am using VS2005 to build 64 bits binaries :-)

But I also agree that keeping the code simple and clean is very important.

Original comment by [email protected] on 13 Apr 2012 at 11:29

[GoogleCodeExporter]

OK, i'm back after a much needed break.

Please find in attached file a proposed modification of LZ4 covering both the 
32 bits and 64 bits situations.

The patch is now 7 lines.
It is not a lot more than 2 lines, but it's no longer invisible.

And I'm somewhat puzzled by the pending benefit.
If i do understand correctly, it corrects a problem of linking stage within 
Visual Studio 2005 (not later versions), specifically when linking Debug binary 
(not the Release one).
This problem, by the way, is solved by the "/Oi" option, which apparently 
should have been automatically selected by MS Visual to begin with.

It seems to me that it is a quite narrow corner case.
So is it worse 7 lines in the main source trunk ?
Or should it be left to a dedicated fork ?


Your comments are always welcomed

Original comment by [email protected] on 3 May 2012 at 1:57

Attachments:

• lz4.zip

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

The 7 lines version is ok :-)

I think adding '/Oi' argument to debug version is not a good solution, because 
it could be a obstruction for the debugging process (e.g.: step over and step 
into actions will not work correctly).

Original comment by [email protected] on 3 May 2012 at 2:19

[GoogleCodeExporter]

OK,
although this is to support a very specific configuration,
since the change is fairly well located in a single place
i will update LZ4 with the 7-lines patch.

Original comment by [email protected] on 7 May 2012 at 10:58

[GoogleCodeExporter]

Corrected into r63

Original comment by [email protected] on 7 May 2012 at 11:11

• Changed state: Fixed