Hi, I've re-pushed to my gitorious clone.
Brad King wrote: > 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..." Merged to next as a separate branch. > > ----- > > 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). Done. > > 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. Done. Looks like I missed step 3 or 'copy/paste/modify' :) > > 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.". Done. > > Your logic should use GetSafeDefinition instead of GetDefinition before > storing the result in a std::string because the latter returns NULL if > not set. Given that the variable being replaced is fetched as a RequiredDefinition, I used that for now. I guess I can change it, but I think it makes sense as a RequiredDefinition. > > AddPositionIndependentFlags shouldn't care whether the library is > shared or not, right? My vague understanding up to now has been that static libraries should not get the flag: http://www.gnu.org/software/libtool/manual/html_node/Static-libraries.html http://gcc.gnu.org/bugzilla/show_bug.cgi?id=28811 http://stackoverflow.com/questions/1589977/linking-a-shared-library-against- a-static-library-must-the-static-library-be-co I'm not certain though... > 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()); > } Done, though I don't know if static libraries should be excluded here. > > 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. Done. I'm also not completely certain about the change to the try_compile behavior in the branch. Could there be any reason to use set(CMAKE_POSITION_INDEPENDENT_CODE ON) but want to do a try_compile without -fPI{C,E} ? My guess is no, because one would do the try_compile before conditionally setting that property, I guess. Thanks, Steve. -- 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
