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