[cmake-developers] push of LinkOptionsCommand topic branch

[Steve Wilson]

I have applied all the suggestions and re-pushed LinkOptionsCommand (after rebasing) to stage.

Thanks,

SteveW

On Feb 2, 2014, at 2:44 AM, Stephen Kelly <steveire at gmail.com> wrote:

> Steve Wilson wrote:
> 
>> I have just pushed the LinkOptionsCommand topic branch to stage.   This
>> topic branch contains commits that implement support for:
>> 
>> add_link_options()
>> 
>> target_link_options()
>> 
>> and the LINK_OPTIONS variable.
>> 
>> Please take a look as interested and send me feedback.
> 
> 
> Thanks for that. 
> 
> 1) The first thing I notice is that I don't think you've broken the commits 
> up along the correct fault lines. It would make more sense to squash the 
> changes to cmTarget, cmLocalGenerator, the generators, the DAGChecker and 
> the cmTargetLinkOptionsCommand together along with the help for that command 
> and target properties and the tests for it. Actually, instead of squashing 
> it into one commit, you can consider creating multiple commits, eg one which 
> adds the {INTERFACE_,}LINK_OPTIONS props (and tests and documents them), and 
> another which adds the command (with tests and docs). Consider how you can 
> split your commits along 'interface changes' 'fault lines'.
> 
> In follow-up commits you can add the cmAddLinkOptionsCommand with the 
> changes to the cmMakefile (and tests and docs), and exporting (with test and 
> docs extensions).
> 
> That order of commits makes it clear what the dependent changes are. The 
> cmAddLinkOptionsCommand and changes to the cmMakefile are convenience only. 
> The relevant change to CMake is support for the target property, and the 
> high-level way to set that target property is cmTargetLinkOptionsCommand. 
> 
> As it is, your first commit adds changes to the cmMakefile interface but no 
> users of the new AddLinkOption interface until the command is added. That's 
> why that change should be in the same commit as the new command.
> 
> The commit message would then describe changes relevant to the user 
> interface, instead of only specific changes to the class interfaces, which 
> probably don't belong in the commit message anyway.
> 
> If you don't know how to rebase with git, just leave all of this for now.
> 
> 2) The processLinkOptionsInternal method is almost identical to 
> processCompileOptionsInternal. Consider renaming the latter (in a separate, 
> standalone commit), refactoring it and using it instead.
> 
> 3) Your topic doesn't remove code from the generators which reads the 
> LINK_FLAGS target property. As the new cmLocalGenerator method will do that, 
> you can remove it from the concrete generators. See eg commit 35496761 where 
> I did similar for COMPILE_FLAGS.
> 
> 4) The use of PROCESS_BEFORE in cmTargetCompileOptionsCommand seems to be a 
> bug, don't repeat it in cmTargetLinkOptionsCommand.
> 
> 5) The commit which adds exporting of the new target property should extend 
> the ExportImport unit test.
> 
> 6) New docs should link to target properties with rst code like 
> :prop_tgt:`LINK_OPTIONS`. Some of the COMPILE_OPTIONS related doc is too old 
> to link like this properly, so see Help/manual/cmake-buildsystem.7.rst for 
> example.
> 
> 7) Consider extending Help/manual/cmake-buildsystem.7.rst with notes about 
> setting the LINK_OPTIONS and new commands.
> 
> 8) One of the reasons I didn't add LINK_OPTIONS before was that I didn't 
> know whether it should accept "--no-undefined" or "-Wl,--no-undefined", or 
> both. Is the latter compiler-driver-specific? Is that irrelevant because the 
> link option is tool specific anyway?
> 
> 9) Use spaces, not tabs, even in tests.
> 
> I've listed a lot of feedback, but it's all minor stuff. Thanks for the 
> contribution.
> 
> Steve.
> 
> 
> 
> -- 
> 
> 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

-------------- 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/20140203/003d4769/attachment-0002.sig>