mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "haosdent huang" <haosd...@gmail.com>
Subject Re: Review Request 37502: Add CMAKE_CXX_FLAGS to GMOCK_CONFIG_CMD in CMake.
Date Mon, 14 Sep 2015 19:21:47 GMT


> On Sept. 11, 2015, 7:25 p.m., Alex Clemmer wrote:
> > 3rdparty/libprocess/3rdparty/CMakeLists.txt, line 166
> > <https://reviews.apache.org/r/37502/diff/1/?file=1041030#file1041030line166>
> >
> >     So, what's the rationale for using `${CMAKE_CXX_FLAGS}` here? It seems to me
that we only want the flags that GTest and GMock need. Is that right? What am I missing?
> >     
> >     Additionally, do we wonly want to set `GTEST_LANG_CXX11` in the case that we're
compiling on Clang? If so, why?
> 
> Alex Clemmer wrote:
>     To add specific recommendations here, assuming the issues I've raised are correct:
>     
>     1. I recommend we remove of the `${CMAKE_CXX_FLAGS}` from the `GMOCK_CONFIG_CXXFLAGS`
variable.
>     2. I recommend we remove `-DGTEST_USE_OWN_TR1_TUPLE` anywhere we are passing it in
(including the autotools build system), as it seems GMock 1.7 makes it irrelevant.
>     3. I recommend we move `-DGTEST_LANG_CXX11` out of the `if (APPLE)` tags, as there
does not seem to be rationale about why it should only be compiled as C++11 on Appleclang.
(If this is not correct, I recommend we add a comment explaining why it is not correct.)

> ${CMAKE_CXX_FLAGS}

${CMAKE_CXX_FLAGS} is used for pass CXX_FLAGS from outsite CMakeLists.txt. Like this one in
MesosConfigure.cmake:
```
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++11")
```

> -DGTEST_LANG_CXX11

I think it still necessary for clang to use c++11 to compile gtest. https://github.com/google/googlemock/blob/release-1.7.0/include/gmock/gmock-matchers.h#L56
and  https://github.com/google/googletest/blob/release-1.7.0/include/gtest/internal/gtest-port.h#L272

> -DGTEST_USE_OWN_TR1_TUPLE

I think we want to make sure the consistency (always use google tuple) in different environments
before. We could remove it in autotools and cmake because you metion it has already set intelligently.


- haosdent


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


On Sept. 4, 2015, 4:25 a.m., haosdent huang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37502/
> -----------------------------------------------------------
> 
> (Updated Sept. 4, 2015, 4:25 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Alex Clemmer, and Joseph Wu.
> 
> 
> Bugs: MESOS-3270
>     https://issues.apache.org/jira/browse/MESOS-3270
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Add CMAKE_CXX_FLAGS to GMOCK_CONFIG_CMD in CMake.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/CMakeLists.txt 997cc0d0e316e316136d4746e50e9e292a82b36b

> 
> Diff: https://reviews.apache.org/r/37502/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> haosdent huang
> 
>


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