[cmake-developers] push of LinkOptionsCommand topic branch

[Stephen Kelly]

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.