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