On 06/06/2013 07:40 AM, Stephen Kelly wrote:
> Ok, I've updated the topic in my clone. I think the only remaining issue is 
> new tests for tll().

Thanks.  Here are some comments:

+    "Enable use of the INTERFACE_LINK_LIBRARIES property.",

How about "INTERFACE_LINK_LIBRARIES defines the link interface.".
The wording needs to make sense when viewed in the future when
everyone is used to this behavior.  It should not sound like
this is an option one must "enable".

+    "The OLD behavior for this policy is to use the INTERFACE_LINK_LIBRARIES "
+    "property only for IMPORTED targets, and ignore it otherwise.  "
+    "The NEW behavior for this policy is to use the INTERFACE_LINK_LIBRARIES "
+    "property for all targets, and ignore the old properties matching"
+    "(IMPORTED_)?LINK_INTERFACE_LIBRARIES(_<CONFIG>)?.",

The policy should not affect imported targets at all.
Even mentioning "(IMPORTED_)?" in the documentation may
be confusing.  We should just document that the policy is only
for in-project targets.  Then over in the property documentation
for the old and new properties document when one overrides the
other.

The wording of the INTERFACE_LINK_LIBRARIES documentation looks
too much like LINK_INTERFACE_LIBRARIES and talks about "replacing
the default".  That's not quite how the new property works,
especially after the tll signature policy.  Please revise this.

+      "The EXPORT_LINK_INTERFACE_LIBRARIES keyword, if present, causes the "
+      "contents of the LINK_INTERFACE_LIBRARIES properties to be exported, "
+      "even if policy CMP0022 is NEW.  "

s/even if/when/ since the keyword cannot be used without the policy.
Also use the wording "properties matching ..." to refer to the
properties more precisely.

+    "with downstream users of older cmake versions."

We should specify the CMake versions: < 2.8.12.

The tll signature commit does not do what I had in mind.  The
signature policy should not affect availability of signatures,
only the combinations allowed.  I expected to see things like

- else if(args[i] == "LINK_PUBLIC")
+ else if(args[i] == "PUBLIC" || args[i] == "LINK_PUBLIC")

for the option aliases.  Also the signature policy should not
affect whether tll populates the old or new link interface
properties.  Only the other policy should do that.  I also
expected to see modification of tll as part of introduction
of the link interface policy.

Like I said before, these policies are orthogonal.  The signature
policy should be completely implementable before introduction
of the new interface properties.  I think your topic will be
simpler to implement and review if you do the signature policy
first to the point that we can merge it to master and then base
the INTERFACE_LINK_LIBRARIES part on that.

Thanks,
-Brad
--

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

Reply via email to