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

Add warnings control #216

Merged
merged 4 commits into from Aug 9, 2017

Conversation

Projects
None yet
4 participants
@opilar
Copy link
Contributor

opilar commented Jul 19, 2017

Resolves #190

src/lib.rs Outdated
ToolFamily::Msvc => &MSVC_FLAGS,
ToolFamily::Gnu => &GNU_FLAGS,
ToolFamily::Clang => &CLANG_FLAGS,
}

This comment has been minimized.

@opilar

opilar Jul 19, 2017

Author Contributor

Can it be simpler?

This comment has been minimized.

@ronnychevalier

ronnychevalier Jul 20, 2017

Contributor

I am not sure using -Wpedantic is a good idea... It rejects (an error not a warning) some programs using non standard features. Since this is by default it will break many crates using gcc-rs since it is not common for C code to strictly follow ISO C standard.

Also, using -Weverything with clang is not necessarily a good idea either. There is some warnings that do not make sense in some programs. It will just spam the output when compiling for no good reason and it will probably get people used to see warnings, thus they will ignore them...

This comment has been minimized.

@opilar

opilar Aug 8, 2017

Author Contributor

Thank you!

@alexcrichton

This comment has been minimized.

Copy link
Owner

alexcrichton commented Jul 20, 2017

Thanks! Could you be sure to add some tests for this as well?

Also, is it worth having different sets of default warnings for gcc vs clang?

ToolFamily::Msvc => "/WX",
ToolFamily::Gnu |
ToolFamily::Clang => "-Werror"
}

This comment has been minimized.

@ronnychevalier

ronnychevalier Jul 20, 2017

Contributor

I think turning warnings into errors only make sense if you are a developer of the crate using gcc-rs (i.e., enabling the warnings as errors only in a "developer mode" or something). Any user of this crate, or any other crate depending on it, could fail during compile time. Some warnings only appear on some architecture. Some people might have a version of the compiler that have a new warning, but not the developers. Hence, if the developers use another architecture or another compiler they will not have any warning, thus no error. But another person might have the error.

I think it would make sense to put some comment on the risk of using this function in your build.rs?

What do you think?

This comment has been minimized.

@alexcrichton

alexcrichton Jul 20, 2017

Owner

Sounds fine by me!

@opilar opilar force-pushed the opilar:feature/warning-control branch from d5161c6 to 3933e00 Aug 8, 2017

@alexcrichton alexcrichton merged commit c74cd42 into alexcrichton:master Aug 9, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@alexcrichton

This comment has been minimized.

Copy link
Owner

alexcrichton commented Aug 9, 2017

Thanks!

@@ -91,6 +91,8 @@ pub struct Config {
shared_flag: Option<bool>,
static_flag: Option<bool>,
check_file_created: bool,
warnings_into_errors: bool,
warnings: bool,

This comment has been minimized.

@gnzlbg

gnzlbg Apr 27, 2018

Contributor

I'd like to be able to turn off -Wextra only, many projects compile with -Wall just fine, but they don't compile silently with -Wextra, and this is on purpose.

@gnzlbg

This comment has been minimized.

Copy link
Contributor

gnzlbg commented Apr 27, 2018

Why was -Wextra added here? -Wextra is basically the set of warnings that aren't good enough to be enabled by default, due to either false-positives, or because they are opinionated. Enabling them for all projects by default, and providing only a way to either use -Wall -Wextra or no warnings at all, does not appear to be reasonable behavior to me.

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.