[cmake-developers] cmake -E capabilities [attempt 2]

[Stephen Kelly]

Tobias Hunger wrote:

> Hi Stephen,
> 
> thanks for taking the time to do such a thorough review!

Actually my review wasn't thorough at all. I didn't try to review it 
further. 

The NewFactory methods in your patch don't return a new'd object, but 
instead return static locals. The regular generators NewFactory methods 
don't work that way, so you're introducing a pattern which is different to 
what already exists and the commit message doesn't say why. Maybe there's a 
reason, but I don't know what it is. In the future, no one else will know 
either.

There are lots of things in there for which your intent is unclear. That's 
why splitting and writing descriptive commit messages is valuable. You might 
find 

 https://vimeo.com/172882423

to be a good introduction to this.

I have more to ask about your first commit (and why the second commit is 
split out). I stopped my review at recommending it be split to see what it 
is hiding. 

> Added const to some of them:-) Hope I caught all.

cmake::ReportCapabilities() should be const, right?

>> * and a whitespace change that should be squashed into the commit that
>> introduces it
> 
> I used Utilities/Scripts/clang-format.bash to do the formatting, so
> that should not be an issue. I just reran that on all commits. Maybe I
> forgot it in a commit or something before.

Maybe. Running the script on all commits would be the fix anyway.

>> * The CMAKE_BUILD_WITH_CMAKE macro should be in cmcmd.cxx wrapping the
>> capabilities handling:
>>
>> #if defined(CMAKE_BUILD_WITH_CMAKE)
>>     else if (args[1] == "capabilities") {
>>       cmake cm;
>>       std::cout << cm.ReportCapabilities();
>>       return 0;
>>     }
>> #endif
> 
> Why?

Because it's a bit odd to return 0 if capabilities can't be reported. I 
missed that the contents of that method don't compile in bootstrap mode 
though without the define.

>> As it is, it is doing many different things, none of which are mentioned
>> in the commit message, and some of which it probably shouldn't be doing.
>>
>> For example renaming GetExtraGeneratorName to
>> GetExternalMakefileProjectGeneratorName is probably not needed. If you
>> really want to do it, then it should be in its own commit with its own
>> commit message which justifies the change. As it is, it adds noise to the
>> big commit and makes it harder to review. Minimal is always better with
>> commits which do refactoring like that.
> 
> I undid that change. That is one of the things that I originally
> removed and then realized last minute that it is needed somehow. So I
> added it, not realizing that I had removed similar functionality
> earlier.

This doesn't happen if you split commits. Splitting makes these things 
visible to you and you can decide whether they were intentional or not at 
that point.

>> A general good rule of thumb (which helps reviews, and makes things
>> bisectable in the future) is to do one thing per commit.
> 
> I agree that this is the ideal we all should all strive for, but you
> are not going to get that from me anytime soon. 

Calling it an 'ideal' isn't really the right mindset. It's easy, and it's 
generally done by all other cmake contributors.

> At least not in the cmake codebase.

I don't know what this part means. It seems somehow disdainful, but I might 
be missing something. Something to talk about in person perhaps.

Thanks,

Steve.