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