On 05/17/2012 08:36 AM, Stephen Kelly wrote:
> Done and re-pushed to my clone. I still have to write the unit tests, but I
> think it can be reviewed again at this point.
Great start. Here are some comments.
The "Update the documentation of IMPORTED_LOCATION to match the code." commit
has an incorrect message. You're modifying the MAP_IMPORTED_CONFIG_<CONFIG>
docs. Also, the original wording the commit changes is correct under the
assumption that the property is set, and if the property is not set then why
should its docs apply? Anyway, I think it can be made even more clear than
your proposed wording by using
"If this property is set and no matching configurations..."
-----
The LanguageToCachedSharedLibFlags ivar would be better named
LanguageToOriginalSharedLibFlags IMO. Also please add a comment
where they are set to explain why (just ref the policy).
The proposed CMP0018 summary should prefer "POSITION_INDEPENDENT_CODE"
instead of "COMPILE_OPTIONS", no? The property is part of the public
interface the user can set/get. The COMPILE_OPTIONS values are impl
details. Also the documentation appears to be cut-n-pasted from another
policy. Please fill in the details.
The docs of POSITION_INDEPENDENT_CODE are incorrect. The last sentence
should be "This property is true by default for SHARED and MODULE library
targets and false otherwise.".
Your logic should use GetSafeDefinition instead of GetDefinition before
storing the result in a std::string because the latter returns NULL if
not set.
AddPositionIndependentFlags shouldn't care whether the library is
shared or not, right? If the property is true then we want the flags.
Perhaps:
std::string picFlags;
if (targetType == cmTarget::EXECUTABLE)
{
std::string flagsVar = "CMAKE_";
flagsVar += lang;
flagsVar += "_COMPILE_OPTIONS_PIE";
picFlags = this->Makefile->GetSafeDefinition(flagsVar.c_str());
}
if(picFlags.empty())
{
std::string flagsVar = "CMAKE_";
flagsVar += lang;
flagsVar += "_COMPILE_OPTIONS_PIC";
picFlags = this->Makefile->GetSafeDefinition(flagsVar.c_str());
}
if(!picFlags.empty())
{
this->AppendFlags(flags, picFlags.c_str());
}
There is a lot more to adding a policy than just
GetPolicyStatus(cmPolicies::CMP0018) == cmPolicies::OLD
Look at some of the other policies. You need to handle all the possible
values. If it is WARN then you need to produce the warning and treat as
OLD behavior. See the switch() statements used for others.
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
