I have implemented all the suggestions from the list below (modulo number 5 which needs more input from others). I have pushed the new branch to stage.
SteveW On Feb 4, 2014, at 3:41 AM, Stephen Kelly <steve...@gmail.com> wrote: > 1) Your first commit should be split up into at least two commits. > > The first commit in your topic should probably refactor the generators to > use a new cmLocalGenerator::AddLinkOptions method which uses the LINK_FLAGS. > See eg commit 35496761 where I did similar for COMPILE_FLAGS. The > AddLinkOptions will have the special handling of OBJECT_LIBRARY and > STATIC_LIBRARY from the Xcode generator. If that is appropriate for all > generators, the commit message should say why. > > The second commit should add the COMPILE_OPTIONS handling to that method. > > My request that you create commits which change the user interface along > with all supporting code to do so may have been confusing. Really what is > needed is to create commits which are complete, self-contained and doing one > thing at a time. That's why it makes sense to split the first commit up into > two parts. If it makes sense to split it into further self-contained and > complete parts, feel free to do so. I just wanted to discourage commits > which are divided up by 'changes to files' rather than 'conceptual changes'. > > For example, changing processCompileOptionsInternal to > processOptionsInternal and changing the logName at call sites could be a > standalone commit preceeding your first commit. > > 2) The change to cmNinjaNormalTargetGenerator seems incorrect. Should > linkFlags should be used with AddLinkOptions? > > 3) Documentation title underlines should be as long as the title, not 3 > dashes longer. > > 4) Tests should avoid bad practices like using target_link_options to add > libraries. Instead try to choose suitable link flags for the tests. > > On GNU, you can do this: > > add_executable(foo foo.cpp) > target_link_options(foo PRIVATE -Wl,--ignore-unresolved-symbol,main) > > and write a foo.cpp which does not define main. > > 5) Regarding what I said before about accepting -Wl,--no-undefined versus > accepting --no-undefined, I think the case of flags with arguments as above > shows that only the -Wl, prefixed case should be accepted (as your branch > already does). > > The documentation should possibly be clear that the contents of LINK_OPTIONS > should be suitable for passing to the driver, not directly to the linker. > > 6) For the ExportImport test, you should create some export target on the > Export side, and use it on the Import side. For example, on the Export side, > do something like > > add_library(no_main_is_ok INTERFACE) > target_link_options(no_main_is_ok > INTERFACE -Wl,--ignore-unresolved-symbol,main) > # ... Export the target. > > and on the Import side, > > add_executable(exe_no_main exe_no_main.cpp) > target_link_libraries(exe_no_main no_main_is_ok) > > That should be done for both the import from the build tree and the install > tree, as much of the existing code in Import/ does. > > 7) I've added two commits to your branch. Please squash them into the > appropriate commits in your topic.
signature.asc
Description: Message signed with OpenPGP using GPGMail
-- Powered by www.kitware.com Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Follow this link to subscribe/unsubscribe: http://public.kitware.com/cgi-bin/mailman/listinfo/cmake-developers