On 07/08/2015 11:21 AM, Mikhail Filimonov wrote:
> I've extended the Nsight Tegra project generator in CMake and added a bunch
> of properties with the backing variables to fine-tune the generated projects.

Thanks!  Here are some comments.

> -        --build-options -DCMAKE_SYSTEM_NAME=Android
> +        --build-options -DCMAKE_SYSTEM_NAME=Android -DCMAKE_TOOLCHAIN_FILE= 
> "${CMake_SOURCE_DIR}/Tests/VSNsightTegra/NsightTegraToolchain.cmake"
[snip]
> +# CMake toolchain file used for the generation of Nsight Tegra project files
> +
> +set(CMAKE_GENERATOR_TOOLSET clang-3.5)
> +set(CMAKE_SYSTEM_NAME Android)

We'd like to keep testing the default behavior without an explicit
toolchain file.  Also we need to be independent of the toolsets
that happen to be available on the system where the test runs,
so we should avoid hard-coding a toolset name if possible.  Is
this part of the patch needed to test everything else?  If so,
please look at adding more test cases for the new combinations
(which can just be more builds of the same test source tree).

> +set_property(TARGET twolib-second APPEND PROPERTY 
> ANDROID_NATIVE_LIB_DIRECTORIES c:\\wrk c:\\wrk\\cmake)

The test should not name directories outside control of the test
suite.  Please name directories in the test source tree or something.

Also, in Source/* C++ sources please wrap lines to keep them <= 79
columns.

Thanks,
-Brad
-- 

Powered by www.kitware.com

Please keep messages on-topic and check the CMake FAQ at: 
http://www.cmake.org/Wiki/CMake_FAQ

Kitware offers various services to support the CMake community. For more 
information on each offering, please visit:

CMake Support: http://cmake.org/cmake/help/support.html
CMake Consulting: http://cmake.org/cmake/help/consulting.html
CMake Training Courses: http://cmake.org/cmake/help/training.html

Visit other Kitware open-source projects at 
http://www.kitware.com/opensource/opensource.html

Follow this link to subscribe/unsubscribe:
http://public.kitware.com/mailman/listinfo/cmake-developers

Reply via email to