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.

* 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"

* language-generator-expression: based on join-genex, see above

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

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

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

Reply via email to