[cmake-developers] Review request: set_emptyvar_PARENT_SCOPE

[Brad King]

On 11/11/2013 05:13 AM, Daniele E. Domenichelli wrote:
> I've pushed a patch to the set_emptyvar_PARENT_SCOPE topic.

The first commit that adds a period to the summary of CMP0039
is a separate change, so I've taken that out into a separate
'policy-summary-periods' topic which is now in 'next'.  Thanks.

Here are a few comments on the main change:

+  this->DefinePolicy(
+    CMP0040, "CMP0040",
+    "set(var "" PARENT_SCOPE) no longer unset var.",
+    3,0,0,0, cmPolicies::WARN);

You need to escape the quotes inside the string here.

This hunk is important:

+  // SET (VAR PARENT_SCOPE) // Removes the definition of VAR
+                            // in the parent scope.
+  else if (args.size() == 2 && args[args.size()-1] == "PARENT_SCOPE")
+    {
+    this->Makefile->RaiseScope(variable, 0);
+    return true;
+    }

and should be taken regardless of the decision below.

One problem with the policy is that it warns more than necessary.
The difference between set-to-empty and unset is only visible in
"if(DEFINED VAR)" tests and not in "${VAR}".  Implementing a policy
that warns only when there will actually be a difference in behavior
will be very difficult.  Also the combination of code needed to
observe a difference in behavior is quite obscure.

In the current behavior:

 set(VAR)              # same as unset(VAR)
 set(VAR PARENT_SCOPE) # same as unset(VAR PARENT_SCOPE)
 set(VAR "")           # sets to empty

and the documentation agrees.  Furthermore, the case in question

 set(VAR "" PARENT_SCOPE)

is documented as setting to empty in the parent scope already.
It is only the implementation that is buggy.  I think we can assume
that anyone who writes this code intends to set to empty rather than
unset in the parent scope, and is likely using just ${VAR} and does
not know the difference.

Therefore I think we can actually skip the policy in this case and
simply treat this as a bug fix, especially given that the change will
come with a major version number.

-Brad