You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@mesos.apache.org by Andrew Schwartzmeyer <an...@schwartzmeyer.com> on 2018/03/09 22:38:19 UTC

Review Request 66008: CMake: Enabled compiler warnings.

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

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

Re: Review Request 66008: CMake: Enabled compiler warnings.

Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.


> On March 19, 2018, 6:35 a.m., Benjamin Bannier wrote:
> > src/CMakeLists.txt
> > Lines 484-486 (patched)
> > <https://reviews.apache.org/r/66008/diff/2/?file=1975960#file1975960line484>
> >
> >     Let's make this a `PRIVATE` option. We wouldn't necessarily want to push this on each and every user.

Hm, then we should at least add it to `mesos-tests` too. I might right a function `ENABLE_WERROR(<target>)` to avoid duplicating this code over and over if we want to keep it private. Sound good?


- Andrew


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


On March 14, 2018, 4:10 p.m., Andrew Schwartzmeyer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66008/
> -----------------------------------------------------------
> 
> (Updated March 14, 2018, 4:10 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Benjamin Bannier, Jeff Coffler, John Kordich, Joseph Wu, and Michael Park.
> 
> 
> Bugs: MESOS-8033 and MESOS-8658
>     https://issues.apache.org/jira/browse/MESOS-8033
>     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.
> 
> Because `-Werror` semantics can be burdensome, it can be turned off at
> configuration time with `-DENABLE_WERROR=FALSE`. It is on by default.
> 
> This also replaces the use of `string(APPEND CMAKE_CXX_FLAGS)` with
> the canonical command `add_compile_options`. Although generally the
> use of `target_compile_options` is preferred, it would currently
> result in a lot more churn, and the build already supports setting
> these flags globally.
> 
> 
> Diffs
> -----
> 
>   cmake/CompilationConfigure.cmake efee36c1ffda096a97af23d481fc0d0903124e54 
>   docs/configuration/cmake.md 1e34657ea55fa324f65f865f7d0a67084c6719d9 
>   src/CMakeLists.txt 0c135034982a6a4844509cd0e9b2cc41804333ef 
> 
> 
> Diff: https://reviews.apache.org/r/66008/diff/2/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 66008: CMake: Enabled compiler warnings.

Posted by Benjamin Bannier <be...@mesosphere.io>.

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


Fix it, then Ship it!




Looking good, could not verify Windows-related changes.


src/CMakeLists.txt
Lines 484-486 (patched)
<https://reviews.apache.org/r/66008/#comment279741>

    Let's make this a `PRIVATE` option. We wouldn't necessarily want to push this on each and every user.


- Benjamin Bannier


On March 15, 2018, 12:10 a.m., Andrew Schwartzmeyer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66008/
> -----------------------------------------------------------
> 
> (Updated March 15, 2018, 12:10 a.m.)
> 
> 
> Review request for mesos, Akash Gupta, Benjamin Bannier, Jeff Coffler, John Kordich, Joseph Wu, and Michael Park.
> 
> 
> Bugs: MESOS-8033 and MESOS-8658
>     https://issues.apache.org/jira/browse/MESOS-8033
>     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.
> 
> Because `-Werror` semantics can be burdensome, it can be turned off at
> configuration time with `-DENABLE_WERROR=FALSE`. It is on by default.
> 
> This also replaces the use of `string(APPEND CMAKE_CXX_FLAGS)` with
> the canonical command `add_compile_options`. Although generally the
> use of `target_compile_options` is preferred, it would currently
> result in a lot more churn, and the build already supports setting
> these flags globally.
> 
> 
> Diffs
> -----
> 
>   cmake/CompilationConfigure.cmake efee36c1ffda096a97af23d481fc0d0903124e54 
>   docs/configuration/cmake.md 1e34657ea55fa324f65f865f7d0a67084c6719d9 
>   src/CMakeLists.txt 0c135034982a6a4844509cd0e9b2cc41804333ef 
> 
> 
> Diff: https://reviews.apache.org/r/66008/diff/2/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 66008: CMake: Enabled compiler warnings.

Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.

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

(Updated March 19, 2018, 12:36 p.m.)


Review request for mesos, Akash Gupta, Benjamin Bannier, Jeff Coffler, John Kordich, Joseph Wu, and Michael Park.


Changes
-------

Removed -Werror semantics, to be done later.


Bugs: MESOS-8033 and MESOS-8658
    https://issues.apache.org/jira/browse/MESOS-8033
    https://issues.apache.org/jira/browse/MESOS-8658


Repository: mesos


Description (updated)
-------

We had previously been using the default sets of warnings, but now we
use the same warnings as on Autotools. This meant disabling 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`. Although generally the
use of `target_compile_options` is preferred, it would currently
result in a lot more churn, and the build already supports setting
these flags globally.


Diffs (updated)
-----

  cmake/CompilationConfigure.cmake efee36c1ffda096a97af23d481fc0d0903124e54 


Diff: https://reviews.apache.org/r/66008/diff/4/

Changes: https://reviews.apache.org/r/66008/diff/3-4/


Testing
-------


Thanks,

Andrew Schwartzmeyer

Re: Review Request 66008: CMake: Enabled compiler warnings.

Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.

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

(Updated March 19, 2018, 12:08 p.m.)

Review request for mesos, Akash Gupta, Benjamin Bannier, Jeff Coffler, John Kordich, Joseph Wu, and Michael Park.

Changes
-------

Addressed Benjamin's comment by making this a 1-1 mapping of the existing `-Werror` semantics. Instead of setting it as public compile option for `libmesos`, we set as a default compile option for the `src` directory recursively.

Bugs: MESOS-8033 and MESOS-8658
https://issues.apache.org/jira/browse/MESOS-8033
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.

Because `-Werror` semantics can be burdensome, it can be turned off at
configuration time with `-DENABLE_WERROR=FALSE`. It is on by default.

This also replaces the use of `string(APPEND CMAKE_CXX_FLAGS)` with
the canonical command `add_compile_options`. Although generally the
use of `target_compile_options` is preferred, it would currently
result in a lot more churn, and the build already supports setting
these flags globally.

Diffs (updated)
-----

cmake/CompilationConfigure.cmake efee36c1ffda096a97af23d481fc0d0903124e54
docs/configuration/cmake.md 1e34657ea55fa324f65f865f7d0a67084c6719d9
src/CMakeLists.txt fb9e9d4cfc4a62830fe3065a139ae14401c0e52e

Diff: https://reviews.apache.org/r/66008/diff/3/

Changes: https://reviews.apache.org/r/66008/diff/2-3/

Testing
-------

Thanks,

Andrew Schwartzmeyer

Re: Review Request 66008: CMake: Enabled compiler warnings.

Posted by Joseph Wu <jo...@mesosphere.io>.

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


Ship it!




Ship It!

- Joseph Wu


On March 14, 2018, 4:10 p.m., Andrew Schwartzmeyer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66008/
> -----------------------------------------------------------
> 
> (Updated March 14, 2018, 4:10 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Benjamin Bannier, Jeff Coffler, John Kordich, Joseph Wu, and Michael Park.
> 
> 
> Bugs: MESOS-8033 and MESOS-8658
>     https://issues.apache.org/jira/browse/MESOS-8033
>     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.
> 
> Because `-Werror` semantics can be burdensome, it can be turned off at
> configuration time with `-DENABLE_WERROR=FALSE`. It is on by default.
> 
> This also replaces the use of `string(APPEND CMAKE_CXX_FLAGS)` with
> the canonical command `add_compile_options`. Although generally the
> use of `target_compile_options` is preferred, it would currently
> result in a lot more churn, and the build already supports setting
> these flags globally.
> 
> 
> Diffs
> -----
> 
>   cmake/CompilationConfigure.cmake efee36c1ffda096a97af23d481fc0d0903124e54 
>   docs/configuration/cmake.md 1e34657ea55fa324f65f865f7d0a67084c6719d9 
>   src/CMakeLists.txt 0c135034982a6a4844509cd0e9b2cc41804333ef 
> 
> 
> Diff: https://reviews.apache.org/r/66008/diff/2/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 66008: CMake: Enabled compiler warnings.

Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.

(Updated March 14, 2018, 4:10 p.m.)

Review request for mesos, Akash Gupta, Benjamin Bannier, Jeff Coffler, John Kordich, Joseph Wu, and Michael Park.

Changes
-------

Address feedback.

Bugs: MESOS-8033 and MESOS-8658
https://issues.apache.org/jira/browse/MESOS-8033
https://issues.apache.org/jira/browse/MESOS-8658

Repository: mesos

Description (updated)
-------

Because `-Werror` semantics can be burdensome, it can be turned off at
configuration time with `-DENABLE_WERROR=FALSE`. It is on by default.

Diffs (updated)
-----

Diff: https://reviews.apache.org/r/66008/diff/2/

Changes: https://reviews.apache.org/r/66008/diff/1-2/

Testing
-------

Thanks,

Andrew Schwartzmeyer

Re: Review Request 66008: CMake: Enabled compiler warnings.

Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.

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

(Updated March 14, 2018, 10:57 a.m.)


Review request for mesos, Akash Gupta, Benjamin Bannier, Jeff Coffler, John Kordich, Joseph Wu, and Michael Park.


Changes
-------

Add other bug.


Bugs: MESOS-8033 and MESOS-8658
    https://issues.apache.org/jira/browse/MESOS-8033
    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

Re: Review Request 66008: CMake: Enabled compiler warnings.

Posted by John Kordich via Review Board <no...@reviews.apache.org>.

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


Ship it!




Ship It!

- John Kordich


On March 9, 2018, 10: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, 10: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
> 
>

Re: Review Request 66008: CMake: Enabled compiler warnings.

Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.


> 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
> 
>

Re: Review Request 66008: CMake: Enabled compiler warnings.

Posted by Benjamin Bannier <be...@mesosphere.io>.

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




cmake/CompilationConfigure.cmake
Lines 220 (patched)
<https://reviews.apache.org/r/66008/#comment279470>

    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` :/



cmake/CompilationConfigure.cmake
Lines 224 (patched)
<https://reviews.apache.org/r/66008/#comment279471>

    Nit: let's put these each flag on a separate line already now, it'll be much nicer to evolve over time.



cmake/CompilationConfigure.cmake
Lines 226 (patched)
<https://reviews.apache.org/r/66008/#comment279472>

    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.



src/CMakeLists.txt
Lines 485 (patched)
<https://reviews.apache.org/r/66008/#comment279473>

    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.


- Benjamin Bannier


On March 9, 2018, 11: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, 11: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
> 
>