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

this is not c89 #11

Closed
khm opened this Issue Feb 8, 2018 · 12 comments

Comments

Projects
None yet
7 participants
@khm
Copy link

commented Feb 8, 2018

You've got a build system that depends on c++, // comments in your code, and you use the inline keyword in string_view.h, leaving aside the stdint.h includes and uintxx_t types scattered everywhere.

Attempting to compile this even once with -std=c89 would have revealed these problems. I recommend you put, at least,
set(CMAKE_C_FLAGS "-std=c89 ${CMAKE_C_FLAGS}")
in your CMakeLists.txt file.

@ip1981

This comment has been minimized.

@khm

This comment has been minimized.

Copy link
Author

commented Feb 8, 2018

I'm giving them the benefit of the doubt on the C++ file; it looks like that's a hello-world demonstrating how to call the library with idiomatic C++. There's no sane reason for the other mistakes in the context of a supposedly c89 library.

@gchatelet

This comment has been minimized.

Copy link
Collaborator

commented Feb 8, 2018

@khm We target gnu89 and not c89. Do you have a real need for c89? Which compiler wouldn't compile this code?

@ip1981 indeed this is a hello world. It's not part of the library.

@khm

This comment has been minimized.

Copy link
Author

commented Feb 8, 2018

I work in a couple of systems that don't support anything past c89, but if you target gnu89, I wouldn't expect this to work.

I came here because I read a blog post [1] that told me this was a c89 library. The same blog post, incidentally, decried hard-coded cpu mapping tables as 'desperate measures.' When I got to the github page I saw the project summary, which currently reads

A cross platform C89 library to get cpu features at runtime.

I was surprised to check out the source code and find that it boils down to an incomplete hard-coded set of cpu mappings, which doesn't compile under c89.

I recommend you update the publicity to reflect at least the coding standards.

[1] https://opensource.googleblog.com/2018/02/cpu-features-library.html

@f1nalspace

This comment has been minimized.

Copy link

commented Feb 8, 2018

At that point where i saw a include of <stdint.h> this library was violating the C89 standard - so you need a recent compiler which support certain features of C99. As i recall gnu89 has some extensions which allows stdint.h for example, but extensions are no standard!
So khm is totally right, this requires a recent compiler - which is fine i think, but then you should declare it as such.

@gchatelet

This comment has been minimized.

Copy link
Collaborator

commented Feb 8, 2018

@khm The blog post was supposed to advertise gnu89 instead of C89. Something went wrong with the publishing. It's unfortunate. Now I understand that my previous answer may have appeared rude, it was not intentional, my apologies for this.

We started out with C89 and ended up picking gnu89 because it was more convenient and we felt this wouldn't affect many people. I'm happy to revisit this design point if many people depend on C89.

I would like to understand your use case a bit more. What cpu features are you trying to detect? May I ask which compiler you use? Which hardware you target?

FYI I'll make sure you don't need a C++ compiler by default (I'll probably rewrite the hello world in C and remove tests from the default build options).

@f1nalspace so you need a recent compiler which support certain features of C99. My definition of recent in this context is about a year old, C99 is definitely a grown up.

@khm

This comment has been minimized.

Copy link
Author

commented Feb 8, 2018

I don't think it's worth rewriting everything not to use uint32_t et al. I see that you updated the project stinger to reflect gnu89 -- but I recommend you stick with C99 instead, because gnu89 is not a standard, and the features this library makes use of were all standardized in C99.

@f1nalspace

This comment has been minimized.

Copy link

commented Feb 9, 2018

@khm Thats exactly what i am talking about. The library should be declared as C99 and then its totally fine.

@gchatelet Yeah the C99 standard is pretty much old, but there are still compilers which does not support them properly (MSVC for example). MSVC does not update the C compiler at all, but they update the C++ compiler to the latest standards and we all know C++/11 has C99 included.

@gchatelet

This comment has been minimized.

Copy link
Collaborator

commented Feb 9, 2018

SGTM I'll advertise C99 then.

@gchatelet gchatelet self-assigned this Feb 9, 2018

@gchatelet gchatelet added the bug label Feb 12, 2018

@gchatelet gchatelet closed this in 00b1c74 Feb 12, 2018

@andlabs

This comment has been minimized.

Copy link
Member

commented Feb 12, 2018

@khm also instead of hardcoding --std=c99, you can set CMAKE_C_EXTENSIONS appropriately (or the equivalent target flag) to turn off extensions.

@Mizux

This comment has been minimized.

Copy link
Collaborator

commented Feb 12, 2018

note: You could use target_compile_features(<target> PUBLIC c_std_99)
Unfortunately it comes with CMake 2.8.2

@tkoeppe

This comment has been minimized.

Copy link

commented Feb 12, 2018

There's also <stdbool.h> which is a C99 feature (as is the _Bool keyword).

gchatelet added a commit that referenced this issue Jan 17, 2019

# This is a combination of 13 commits.
# This is the 1st commit message:

Update macros to detect mips64 and differentiate between x86 32/64.

# This is the commit message #2:

Fix Mips32 and add an alias for Mips32/64.

# This is the commit message #3:

Allow specifying cmake generator.

# This is the commit message #4:

Remove unavailable MSVC version and test Ninja on travis

# This is the commit message #5:

Add ninja build.

# This is the commit message #6:

Use Ninja by default

# This is the commit message #7:

Show tests output

# This is the commit message #8:

Defaulting env to Ninja in the build matrix

# This is the commit message #9:

Defaulting env to Ninja in the build matrix

# This is the commit message #10:

Do not use ninja on OSX

# This is the commit message #11:

Fix test output.

# This is the commit message #12:

Fix running tests

# This is the commit message #13:

Fix running tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.