[cmake-developers] Backward compatbility code

[Brad King]

On 02/10/2014 03:41 AM, Stephen Kelly wrote:
>  diff --git a/Source/cmAddCustomCommandCommand.cxx 
> b/Source/cmAddCustomCommandCommand.cxx
>  index 5634849..e967e3c 100644
>  --- a/Source/cmAddCustomCommandCommand.cxx
>  +++ b/Source/cmAddCustomCommandCommand.cxx
>  @@ -164,6 +164,7 @@ bool cmAddCustomCommandCommand
>            cmSystemTools::ConvertToUnixSlashes(filename);
>            break;
>          case doing_source:
>  +    // TODO: compatibility code. Should this be an error now?

I think all signatures of add_custom_command that use SOURCE can be
disallowed by a policy.

>  @@ -344,6 +345,7 @@ bool cmAddCustomCommandCommand
>      }
>    else
>      {
>  +    // Should AddCustomCommandOldStyle be removed?
>      // Use the old-style mode for backward compatibility.
>      this->Makefile->AddCustomCommandOldStyle(target.c_str(), outputs, 

No.  This supports the non-VERBATIM mode of add_custom_command and
add_custom_target.  There are use cases for both versions.  Perhaps
the internal APIs should be renamed.

>    // cmSourceFileLocation class to commit to a particular full path to
>    // the source file as late as possible.  If the users requests the
>    // LOCATION property we must commit now.
>  +  // Is there something here that can be dropped?
>    if(strcmp(prop, "LOCATION") == 0)
>      {
>      // Commit to a location.

No.  This is just the implementation of the LOCATION source file property
which unlike the LOCATION target property does not depend on the config
so we can keep it.  The big comment is just explaining why we need to
do a GetFullPath there.  The "and backwards compatibility" refers to the
support for naming source files so loosely.  The comment could be reworded.

>  diff --git a/Source/cmTarget.cxx b/Source/cmTarget.cxx
>  index e51095e..86bab95 100644
>  --- a/Source/cmTarget.cxx
>  +++ b/Source/cmTarget.cxx
>  @@ -317,7 +317,8 @@ void cmTarget::SetMakefile(cmMakefile* mf)
>      // variable only for non-executable targets.  This preserves
>      // compatibility with previous CMake versions in which executables
>      // did not support this variable.  Projects may still specify the
>  -    // property directly.  TODO: Make this depend on backwards
>  +    // property directly.
>  +    // TODO: Make this depend on backwards
>      // compatibility setting.

The current non-exe behavior may be desirable because it is common to
add a per-config suffix on libraries but not executables.  The current
behavior is documented in "Help/prop_tgt/CONFIG_POSTFIX.rst" already.
I think we can just remove the TODO comment.

>      if(this->TargetTypeValue != cmTarget::EXECUTABLE
>          && this->TargetTypeValue != cmTarget::INTERFACE_LIBRARY)
>  @@ -589,7 +590,7 @@ cmSourceFile* cmTarget::AddSource(const char* s)
>    std::string src = s;
>  
>    // For backwards compatibility replace varibles in source names.
>  -  // This should eventually be removed.
>  +  // TODO: This should eventually be removed.
>    this->Makefile->ExpandVariablesInString(src);

A policy should be added to remove this behavior.  When the policy is
not set trigger the warning if the expansion changes anything.  I
doubt this is used intentionally in practice anymore.

-Brad