[cmake-developers] A few topic reviews for Stephen

[Stephen Kelly]

Brad King wrote:

> Stephen,
> 
> We reviewed most of your latest topics in 'next' today, merged
> many to 'master', and committed trivial fixups to a couple.
> However, there are problems with some topics:
> 
> * genex-generate-file: Appears to leak memory.  It calls
>   new cmGeneratorExpressionEvaluationFile but has no delete.

Thanks, fixed.

> 
> * join-genex: I think this is the cause of a VS 6 test failure
>   of the CompileDefinitions test:
> 
>    http://open.cdash.org/testDetails.php?test=190944806&build=2911585
>    WARNING: The VS6 IDE does not support preprocessor definition values
>    with spaces and '"', '$', or ';'. CMake is dropping a preprocessor
>    definition: CMAKE_IS_REALLY="Very Fun"

This doesn't seem to be anything to do with the join-genex topic. It was 
present before join-genex was merged to next.

 http://open.cdash.org/testDetails.php?test=157289612&build=2905507

I've pushed a separate topic to fix it.

> * generate_export_header-fixes: See general request/advice below.
>   I updated commit messages in the short topics but this one is
>   longer.  Please revise the commit messages of the entire topic.

Ok. Actually I think this topic shouldn't be merged anyway until the  
VISIBILITY_DEFAULT target property issue is decided. The topic changes the 
add_compiler_export_flags in a way which I would prefer not to do if the 
target property exists (adding a parameter for C flags).

I've pushed the topic to my clone again, so we can remove it from stage for 
now if that helps.

> A general request to make reviews easier: please follow more
> strictly the practice of prefixing commit messages with an
> "area: " header at the beginning of the first line.  Imagine
> reading the commit message without separately knowing the
> list of files it changes.  The "area: " and description should
> together make it possible to infer where the change should be.
> 
> When writing a commit message I like to pretend I'm writing an
> email to someone that instructs them to write the patch.  The
> the commit message summary (first) line is the email subject.
> The rest is the body.  The imperative mood instructs the reader
> to make the change.  When reading a patch it should be plausible
> that it was constructed by a competent developer just from the
> commit message.  This makes review easier because we can check
> if the change does what the message says rather than trying to
> infer the intention and correctness of the change just from the
> patch.

Thanks for the feedback. I'll keep it in mind.

Thanks,

Steve.