[cmake-developers] push of LinkOptionsCommand topic branch

[Steve Wilson]

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 <steveire at 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.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 236 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <http://public.kitware.com/pipermail/cmake-developers/attachments/20140206/f82d7558/attachment-0002.sig>