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