[cmake-developers] Backward compatbility code

[Stephen Kelly]

Brad King wrote:

> 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.

Ok, I'm not sure how to handle these two items. I've never used the SOURCE 
signature of add_custom_command, and the AddCustomCommandOldStyle method 
seems to also relate to things I haven't used before, so I wouldn't know how 
to explain the policy.

> 
>>    // 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.

Ok. I'm not sure what to re-word it to, but it's not a high priority.

> 
>>  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.

Done.

> 
>>      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.

Any idea how to unit test this?

Thanks,

Steve.