[webkit-reviews] review granted: [Bug 84895] [CMake] Rewrite FindCairo.cmake. : [Attachment 139569] Fix the ChangeLog nitpicks

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted Raphael Kubo da Costa (rakuco)
<rakuco at webkit.org>'s request for review:
Bug 84895: [CMake] Rewrite FindCairo.cmake.
https://bugs.webkit.org/show_bug.cgi?id=84895

Attachment 139569: Fix the ChangeLog nitpicks
https://bugs.webkit.org/attachment.cgi?id=139569&action=review

------- Additional Comments from Daniel Bates <dbates at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=139569&action=review


> Source/WebCore/ChangeLog:14
> +	   Change all that by rewriting the module.

It's unclear if "change all that" implies the negation of the "old approach" or
whether the "new approach" is something entirely different.

For your consideration, I suggest writing a short summary of the changes so as
to make it straightforward for someone to understand the "new approach" without
having to read the patch.

> Source/cmake/FindCairo.cmake:51
> +	   STRING(REGEX MATCH "#define +CAIRO_VERSION_MAJOR +([0-9]+)" _dummy
"${CAIRO_VERSION_CONTENT}")

Would it make sense to strengthen this regular expression by matching the start
and/or end of the line?

> Source/cmake/FindCairo.cmake:54
> +	   STRING(REGEX MATCH "#define +CAIRO_VERSION_MINOR +([0-9]+)" _dummy
"${CAIRO_VERSION_CONTENT}")

Ditto.

> Source/cmake/FindCairo.cmake:57
> +	   STRING(REGEX MATCH "#define +CAIRO_VERSION_MICRO +([0-9]+)" _dummy
"${CAIRO_VERSION_CONTENT}")

Ditto.

> Source/cmake/FindCairo.cmake:78
> +IF (Cairo_FIND_VERSION)
> +    IF (Cairo_FIND_VERSION_EXACT)
> +	   IF ("${Cairo_FIND_VERSION}" VERSION_EQUAL "${CAIRO_VERSION}")
> +	       # FIXME: Use IF (NOT ...) with CMake 2.8.2+ to get rid of the
ELSE block
> +	   ELSE ()
> +	       SET(VERSION_OK FALSE)
> +	   ENDIF ()
> +    ELSE ()
> +	   IF ("${Cairo_FIND_VERSION}" VERSION_GREATER "${CAIRO_VERSION}")
> +	       SET(VERSION_OK FALSE)
> +	   ENDIF ()
> +    ENDIF ()
> +ENDIF ()

I wish there was a way to simplify this :(