glassechidna / zxing-cpp

ZXing C++ Library
Apache License 2.0
598 stars 435 forks source link

Fixed declaration in .h file problem with VC++ #38

Closed fschramka closed 8 years ago

fschramka commented 8 years ago

Put all declarations into the .cpp file. Did not work before with VS compiler if you added the static library into your own project.

Error Code was: error LNK2005: "public: static unsigned int const zxing::DecodeHints::CHARACTER_SET" (?CHARACTER_SET@DecodeHints@zxing@@2IB) already defined in MyObject.obj

Problem on stackoverflow (another author): http://stackoverflow.com/questions/18208143/lnk2005-error-with-zxing-c

Benjamin-Dobell commented 8 years ago

Hmmm, I don't understand. Why are there static declarations in a header, I can't think of any case in which that would make sense. EDIT: Ignore this, I was clearly a bit too tired :wink:

What are the compiler errors you were receiving?

Benjamin-Dobell commented 8 years ago

Oops, sorry. Completely misread! I thought those static declarations were outside of the class... Gimme a sec to re-review.

Benjamin-Dobell commented 8 years ago

I'd still be interested to see the actual errors you're receiving, as const primitive variable declarations are allowed in headers.

In addition to that, if you were going to change this I would suggest an enum (in the header), rather than have declarations in the header and definitions in a CPP file; when we're talking about bit-fields it's important to have the values visible in the interface so you can be sure how to safely combine them.

Benjamin-Dobell commented 8 years ago

const DecodeHintType DecodeHints::CHARACTER_SET;

As far as I can tell that's the culprit. Not all the other changes. For some reason that variable is declared twice. Once in a CPP and once in the header. I'd suggest you simply delete it and try again. If it works, please close this pull request and submit a new one.

fschramka commented 8 years ago

My only changes were to put all definition into the .cpp file. I just need the declaration in the header for the lib I imported in another project. Yes, const DecodeHintType DecodeHints::CHARACTER_SET; was the problem. I could just delete it, but have you ever received credits for deleting code ;)? Now it works for sure.

Benjamin-Dobell commented 8 years ago

I could just delete it, but have you ever received credits for deleting code ;)?

Actually, the number of deleted lines is by far my favourite statistic when dealing with an established project. If you can delete code without breaking functionality then you're likely (not always though) giving a project a huge win in terms of readability/maintainability.

Even in this case deleting one line is much better than making a bunch of changes that are unnecessary - because tired people like me get really confused looking at large diffs and make fools of themselves on the Internet! :wink:

In all seriousness though, by moving the definitions into the .cpp file you've actually changed the resolution of the values from compile time to link time. Compilers can't optimise properly at link time. It's probably not going to cause huge performance issues here, but couple that with the disadvantages of having the bitfields removed from the interface, and it's not really a great change to make. Also, moving the definition into the .cpp can cause confusing linker errors (rather than compilation errors) e.g. the one you received regarding DecodeHints::CHARACTER_SET.

I'm going to close this, but please do submit a new pull request with just the offending line removed.