On Sun, Jan 29, 2012 at 11:44 AM, Stephen Kelly <steve...@gmail.com> wrote: > David Cole wrote: >> OK... nearly complete now. Please review, then reply and tell me if >> you object to any of the 7 commits in this topic branch. > > No objections. They all seem fine. >
Great, thanks. >> Steve, I've >> preserved your authorship for most of these commits, but have >> significantly re-written some of them. > > The 3 following 073bc421620e25fef6389b0d8b71cfad8ca79786 (CMake: Eliminate > cmMakefile::IncludeDirectories) seem to have been substantially re-written. > I'm not sure what is the appropriate way to deal with commits like that, but > whatever you choose for the author line is fine. > So I guess the question is: if somebody looks up the author at some point in the future and comes asking a question, do you want to be the one they go to, or would you rather have them come to me? ;-) If you want to send them to me, I'll change the author line, but you certainly deserve the credit for getting the ball rolling on this topic. >> >> I've pushed again (force-pushed, please delete previous checkouts of >> this branch) to github: >> >> https://github.com/dlrdave/CMake/commits/tgt-inc-dirs-vs-xcode >> https://github.com/dlrdave/CMake/commit/0a45e239 >> >> I've verified all tests pass on Mac with Unix Makefiles, Mac with >> Xcode, Windows with VS10 and Windows with VS6. I'm confident we will >> have minimal dashboard issues when merging this to 'next'. > > Great. Thanks for all of your efforts. > >> What remains to be done: >> - need ExpandVariablesInString calls for the target properties, too... >> at >> the same point, or is there a better time to call it? > > Are you asking whether cmMakefile::ExpandVariables() should do something > like (sort-of pseudocode): > > foreach(Target target, this->Targets) > { > const char *includeDirs = target->GetProperty("INCLUDE_DIRECTORIES"); > if (includeDirs) > { > std::string dirs = includeDirs; > this->ExpandVariablesInString(dirs, true, true); > target->SetProperty("INCLUDE_DIRECTORIES", dirs.c_str()); > } > } > > I don't know enough about when ExpandVariables is supposed to be called etc, > but it seems like a sensible place to do it is always called at the correct > point. > > However, it starts to look like ExpandVariablesInString should be API > elsewhere in CMake (cmSystemTools). It's not a very important point though > and doesn't necessarily need to be solved with this branch. > I'll talk this over with Brad and finalize the commits with any changes before pushing this to 'next'. >> - avoid duplicates in "cmMakeDepend::SetMakefile"? > > Considering that it used to call > > this->Makefile->GetIncludeDirectories(); > > which does preserve uniqueness, I think it makes sense to do so with the > ported code too. > Yes, ok, I'll spend the extra effort to make that section a unique list as well. >> - double-check with Alex about the changes in cmake::FindPackage -- >> Alex? > >> - fully remove cmMakefile::GetIncludeDirectories since it has no >> more callers - update documentation for include_directories and the >> INCLUDE_DIRECTORIES property > > I have updated my branch on gitorious with an attempt at documenting the > change (commit 8acd1c07ff299ea823837ba6268b43db92ac81f2). > Thanks -- I'll pull this and use it as a basis for the final topic. David -- 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