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 warning control #190

Closed
brson opened this issue Jul 13, 2017 · 4 comments
Closed

Add warning control #190

brson opened this issue Jul 13, 2017 · 4 comments

Comments

@brson
Copy link
Contributor

brson commented Jul 13, 2017

Do a few things:

  • Turn on maximum warnings by default, in the compiler-specific way
  • Add a method warnings_as_errors that turns warnings into errors in a compiler specific way

We haven't discussed this, but if we enable maximum warnings by default, it seems like some users will not want that and a way to turn it off. So it seems like we should also add a default_warnings method that declines to set the maximum warnings.

@opilar
Copy link
Contributor

opilar commented Jul 16, 2017

How to create a static array and return a slice of it?

   --> src/lib.rs:164:34
    |
164 |             ToolFamily::Msvc => &["/Wall"],
    |                                  ^^^^^^^^-
    |                                  |       |
    |                                  |       temporary value only lives until here
    |                                  does not live long enough
    |
    = note: borrowed value must be valid for the static lifetime...

Method:

    /// What the flags to enable all warnings
    fn warnings_flags(&self) -> &'static [&'static str] {
        match *self {
            ToolFamily::Msvc => &["/Wall"],
            ToolFamily::Gnu => &["-Wall", "-Wpedantic", "-Wextra"],
            ToolFamily::Clang => &["-Weverything"],
        }
    }

@alexcrichton
Copy link
Member

Hm I thought it was like that but I guess not :(. Maybe try making this not a method but instead a temporary in a function?

@kornelski
Copy link
Contributor

kornelski commented Jul 26, 2017

I'm unsure about it.

  1. I mainly use this crate as implementation detail in *-sys crates, and users of my creates have absolutely no control over the code being compiled, so I don't want them to see any warnings they can't fix. Sometimes even I can't fix the warnings myself if I'm repackaging someone else's library. If warnings are on by default, it'll be a thing I always have to remember to turn off.

    I strongly prefer no warnings by default.

  2. Combination of maximal warnings + warnings as errors is super brittle, and shouldn't be allowed. New lints introduced by newer compilers will break builds.

    It's dangerous in general, because different compilers may warn about different things, so authors testing on clang may break crates for MSVC users just because MSVC complains about different things. I remember struggling with basic things like unused variables, where (void)var; works to silence unused warning in one compiler, but in other compiler causes useless expression warning.

@dtolnay
Copy link
Member

dtolnay commented Jul 31, 2017

I agree with @pornel. Warnings enabled seems like the wrong default for the way this crate is usually used, and I would never want to use warnings-as-errors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants