Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[libbladeRF] Header issue in Windows #347

Closed
bpssoft opened this issue Nov 11, 2014 · 9 comments
Closed

[libbladeRF] Header issue in Windows #347

bpssoft opened this issue Nov 11, 2014 · 9 comments
Assignees
Labels
Issue: Bug It's a bug and it oughta be fixed
Milestone

Comments

@bpssoft
Copy link

bpssoft commented Nov 11, 2014

The header file 'libbladerf.h' contains the following line: #include <stdbool.h>
Visual Studio will fail if you are using this header because of the #include <stdbool.h> line.
Can you try to move it inside a #if statement?

@jynik
Copy link
Contributor

jynik commented Nov 11, 2014

Hi there,

I'm not seeing any build issues for 32-bit or 64-bit builds in Visual Studio 2010 and 2012. Could you share a bit more info about your setup?

Could you point out the specific line here (like this) and share the compilation failure message(s)? Thanks!

@bpssoft
Copy link
Author

bpssoft commented Nov 12, 2014

Hi,

The problem is not compiling the source code itself (I use the precompiled version). But when I create my own project with Visual Studio 2012 C++ and try to include “libbladerf.h” I get the following error:
“Error 1 error C1083: Cannot open include file: 'stdbool.h': No such file or directory C:\Program Files\bladeRF\include\libbladeRF.h 26 1 ConsoleTest”
Problem caused by Visual Studio doesn’t support C99 and therefore doesn’t contain stdbool.h
Can you just put this line

into an if statement?

From: Jon Szymaniak [mailto:notifications@github.com]
Sent: dinsdag 11 november 2014 16:59
To: Nuand/bladeRF
Cc: Etienne Goossens - EWI
Subject: Re: [bladeRF] Header issue in Windows (#347)

Hi there,

I'm not seeing any build issues for 32-bit or 64-bit builds in Visual Studio 2010 and 2012. Could you share a bit more info about your setup?

Could you point out the specific line here (like thishttps://github.com/Nuand/bladeRF/blob/5cd6422db756e810a0359ee7523b512be25dc148/host/libraries/libbladeRF/include/libbladeRF.h#L27) and share the compilation failure message(s)? Thanks!


Reply to this email directly or view it on GitHubhttps://github.com//issues/347#issuecomment-62567992.

@jynik jynik closed this as completed Nov 12, 2014
@jynik
Copy link
Contributor

jynik commented Nov 12, 2014

My apologies, I realized I misunderstood your problem statement.

MSVC still supports a bool type, so ifdef'ing out that inclusion of stdbool.h line suffices then? (This doesn't result in any strange issues with respect to the size of the return value?)

As a temporary workaround, you could pull in this stdint.h header.

@jynik jynik reopened this Nov 12, 2014
@jynik
Copy link
Contributor

jynik commented Nov 12, 2014

To elaborate a bit more on my question. The prebuilt DLL is using the stdint.h mentioned, where sizeof(bool) == sizeof(int).

To my knowledge the standards define sizeof(bool) to be implementation-defined (please correct me if I'm wrong). If instead of using the stdint.h header linked above, we ifdef that line out, are we certain that the size of the bool values used by the user's compiler (i.e., whatever bool is without our stdint.h) will be the same as those built in with the DLL (as these are used as return values and fn parameters)?

Without stdint.h, MSVC still supports bool? That would surprise me, and I'll go check later.

For that concern, I'm leaning toward addressing this by shipping our stdint.h along with libbladeRF.h. I'd appreciate any additional feedback and thoughts you have on this.

Thanks!

@bpssoft
Copy link
Author

bpssoft commented Nov 13, 2014

"Without stdint.h, MSVC still supports bool? That would surprise me, and I'll go check later." That is correct. There is no definition for type 'bool' inside stdint.h.
More information http://msdn.microsoft.com/en-us/library/tf4dy80a.aspx

@jynik
Copy link
Contributor

jynik commented Nov 13, 2014

While MSVC supports bool for C++ code as you noted above, it doesn't appear to for C code (which makes sense given that it leans towards C89).

So it looks like we can ifdef that out for C++ code, as you noted. If folks are compiling C code in Visual Studio 2012 or earlier, they'll just have to pull in our stdint.h, which we can ship with the installer from now on. (We'll recommend VS2013 for C users, which is already on the build instructions.)

I see that with MSVC, sizeof(bool) == 1. Therefore our stdint.h header is wrong, and that will have to get fixed as well.

Does this sound reasonable? I'll look to have something in a dev branch soon for your review.

@jynik jynik changed the title Header issue in Windows [libbladeRF] Header issue in Windows Nov 14, 2014
@jynik
Copy link
Contributor

jynik commented Nov 14, 2014

The requested conditional removal of <stdbool.h> and some of the associated items I mentioned here have been applied and staged in a dev-issue_347 branch.

I've made some changes to hopefully help avoid the situation that caused this to go unnoticed for so long. Thanks again, @bpssoft.

Tomorrow I'll verify that this doesn't break things in other build environments and do some additional testing.

If folks really want, I can have a new installer generated and made available. Otherwise, a new one will be up at the release at the end of the month.

@bpssoft
Copy link
Author

bpssoft commented Nov 14, 2014

@jynik Thank you very much for your effort! The solution works as expected.
I've also try to compile the source code to the specific branch and it works fine.
I can also confirm that the compilation on Linux (Mint Linux 17) still works as expected.

I'm really happy to work with bladeRF! Thank you so much for your support

@jynik jynik added the Issue: Bug It's a bug and it oughta be fixed label Nov 14, 2014
@jynik jynik added this to the 2014.11 milestone Nov 14, 2014
@jynik jynik self-assigned this Nov 14, 2014
@jynik
Copy link
Contributor

jynik commented Nov 14, 2014

Merged into master in 7706e92. Removed temporary dev branch.

@jynik jynik closed this as completed Nov 14, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue: Bug It's a bug and it oughta be fixed
Projects
None yet
Development

No branches or pull requests

2 participants