mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrew Schwartzmeyer <and...@schwartzmeyer.com>
Subject Re: Review Request 66008: CMake: Enabled compiler warnings.
Date Wed, 14 Mar 2018 17:56:53 GMT


> On March 14, 2018, 9:29 a.m., Benjamin Bannier wrote:
> > cmake/CompilationConfigure.cmake
> > Lines 220 (patched)
> > <https://reviews.apache.org/r/66008/diff/1/?file=1973215#file1973215line220>
> >
> >     Can we at least add a `TODO` here for what was advertised in the commit message,
> >     
> >     > This also replaces the use of string(APPEND CMAKE_CXX_FLAGS) with
> >     the canonical command add_compile_options (though setting these on a
> >     per-target basis would be even better, it's a separate issue).
> >     
> >     Ideally we would reference a ticket.
> >     
> >     No need to add this everywhere below, but this one sticks out especially since
it says `GLOBAL` :/

I'm sorry, I thought I had. It's [MESOS-8033](https://issues.apache.org/jira/browse/MESOS-8033).
Adding to the review.

And this says `GLOBAL` because `add_compile_options` is a global-ish side-effect. It affects
all targets from "this" directory downward (and while I don't like this design, and think
we should fix it at some point, this configuration file is included in the top-level `CMakeLists.txt`
file, making this essentially global).

Oh wait, I get what you were saying now. So, adding compile options per target instead of
here (and below) is "a separate issue" because it's a design change. It's not something we
necessarily have to or even should do; as these flags are set similarly to the old build system.
It is generally in CMake more appropriate to set flags on a per-target basis, so my commit
message was just calling out why we didn't do that. I'll amend it and make it more clear.


> On March 14, 2018, 9:29 a.m., Benjamin Bannier wrote:
> > cmake/CompilationConfigure.cmake
> > Lines 224 (patched)
> > <https://reviews.apache.org/r/66008/diff/1/?file=1973215#file1973215line224>
> >
> >     Nit: let's put these each flag on a separate line already now, it'll be much
nicer to evolve over time.

;) I went back and forth on this three times. I'm happy to go either way.


> On March 14, 2018, 9:29 a.m., Benjamin Bannier wrote:
> > cmake/CompilationConfigure.cmake
> > Lines 226 (patched)
> > <https://reviews.apache.org/r/66008/diff/1/?file=1973215#file1973215line226>
> >
> >     We definitely do not want to include `-Weverything` ever. It is intended for
testing diagnostics by upstream developers, but would warn about issues we would not want
to address (e.g., struct packing). You probably meant `-Wextra` like in the `GNU` case.

Good to know, thanks.


> On March 14, 2018, 9:29 a.m., Benjamin Bannier wrote:
> > src/CMakeLists.txt
> > Lines 485 (patched)
> > <https://reviews.apache.org/r/66008/diff/1/?file=1973216#file1973216line485>
> >
> >     We should add a cmake configuration flag to disable `-Werror`. We have an equivalent
configure flag `--disable-werror` since it is extremely unlikely that we'll support every
possible setup (e.g., bleeding edge compilers, packaging setups). Let's give users a way to
opt out.

Hm... okay.


- Andrew


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66008/#review199176
-----------------------------------------------------------


On March 9, 2018, 2:38 p.m., Andrew Schwartzmeyer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66008/
> -----------------------------------------------------------
> 
> (Updated March 9, 2018, 2:38 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Benjamin Bannier, Jeff Coffler, John Kordich,
Joseph Wu, and Michael Park.
> 
> 
> Bugs: MESOS-8658
>     https://issues.apache.org/jira/browse/MESOS-8658
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> We had previously been using the default sets of warnings, but now we
> use the same warnings as on Autotools. This also means that we can
> safely turn on the treat-warnings-as-errors (at least for the Mesos
> code). However, doing so requires that we disable two common
> possible-loss-of-data warnings on Windows that are not part of the
> GNU/Clang default warnings.
> 
> This also replaces the use of `string(APPEND CMAKE_CXX_FLAGS)` with
> the canonical command `add_compile_options` (though setting these on a
> per-target basis would be even better, it's a separate issue).
> 
> 
> Diffs
> -----
> 
>   cmake/CompilationConfigure.cmake efee36c1ffda096a97af23d481fc0d0903124e54 
>   src/CMakeLists.txt 0c135034982a6a4844509cd0e9b2cc41804333ef 
> 
> 
> Diff: https://reviews.apache.org/r/66008/diff/1/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message