[cmake-developers] Developer tasks - Refactoring

[Stephen Kelly]

Daniel Pfeifer wrote:

> On Wed, Feb 10, 2016 at 12:12 AM, Stephen Kelly
> <steveire at gmail.com> wrote:
>> 3) Compute cmGeneratorTarget state non-lazily in its constructor.
>> * Historically target state for generators was computed lazily because it
>>   might need to be cleared and re-computed.  That is no-longer true.
> 
> SourceFilesMap is cleared in a call to AddSource. It is then
> recomputed the next time GetSourceFiles is called.

Hmm, maybe there is a way to reduce the amount of times it is cleared, once 
the computation sequence is better defined by being extracted to 
cmComputeTarget. cmGeneratorTarget::AddSource is currently called from 
several generate-time locations, some of which could become compute-time 
locations. The uses in 

* cmQtAutoGeneratorInitializer::InitializeAutogenSources 
* cmGlobalVisualStudio8Generator::AddCheckTarget
* cmLocalVisualStudio7Generator::CreateVCProjBuildRule
* cmGlobalXCodeGenerator

seem that way to me, because the file names don't depend on anything from 
the cmGeneratorTarget which is only known at build time. Unless I'm missing 
something, they could be added to the list of files for the cmComputeTarget  
at compute time before the SourceFilesMap is populated at generate time.

The tricky case is when AddSource is called with something computed from the 
result of cmGeneratorTarget::GetSources. The only case of that seems to be 
SetupSourceFiles in cmQtAutoGeneratorInitializer. That calls AddSource in a 
loop. If cmComputeTarget gains an AddSource and doesn't have a 
SourceFilesMap, then that would be the only use of 
cmGeneratorTarget::AddSource. That could then be replaced with a new 
AddSources or AddSourcesForRCC, or something better might be possible.

This is just a brain-dump which I haven't investigated. You might find some 
facts which make the source addition refactoring to compute time to be 
impossible.

Then maybe the entries of the ComputedTargets map might not have to be 
recomputed lazily.

> Please review:
> https://github.com/purpleKarrot/CMake/commits/computed-target

Cool, thanks for working on this!

I am a bit confused by the first commit in the branch. It removes some c++ 
template code while adding cmComputeTarget. I think that's because you make 
the working class inherit from cmComputeTarget, and you do that just to re-
use the data members, and then later in the branch, to re-use the 
cmComputeTarget itself.

Is it possible to split what is happening in that commit? Is it possible to 
remove the template code first without introducing cmComputeTarget? Even if 
doing so would mean introducing a struct with the same data members early in 
the branch and perhaps removing that struct later, I think it might make the 
commit more clear. Currently I don't understand it.

I also generally recommend to put the most obvious/cleanup commits at the 
start of the branch (or even in a separate minor-cleanups branch). The 
'don't clear container in destructor' commit and the 'use erase-unique 
instead of reinitialization' commit and the 'factor out common part of 
AddSources commands' all seem to fit that description. I was able to rebase 
them at least, but I didn't try building the result. 

Getting those kinds of commits out of the way first makes the rest of the 
branch smaller and focused on extending the code in only one specific way.

Thanks,

Steve.