Bug 143596 – [CMake] Print sorted feature list at the very end of the configure process

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Michael Catanzaro]

Print the list of enabled features at the end of the configure process, instead of in the middle, so that users can actually see it. This is traditional in autotools projects.

Also, sort the list.

[Michael Catanzaro]

Created attachment 250500 [details]
[CMake] Print sorted feature list at the very end

[Michael Catanzaro]

Created attachment 250578 [details]
[CMake] Print sorted feature list at the very end

[Martin Robinson]

Comment on attachment 250578 [details]
[CMake] Print sorted feature list at the very end

View in context: https://bugs.webkit.org/attachment.cgi?id=250578&action=review

Thanks for taking care of the details! This looks good to me, though I have a few picky suggestions.

> Source/cmake/WebKitFeatures.cmake:161
> +    WEBKIT_OPTION_DEFINE(USE_SYSTEM_MALLOC "Toggle system allocator instead of malloc" OFF)

Maybe you could write "WebKit's custom allocator" instead of "malloc." I think "malloc" might be confusing because I think of that as the system allocator. :)

> Source/cmake/WebKitFeatures.cmake:171
> +    # Alphabetize.

Comments in WebKit should really be complete sentences. I'm not sure if a comment is really necessary here though, since list(SORT isn't much less obscure than "Alphabetize."

> Source/cmake/WebKitFeatures.cmake:189
> +        if (NOT _WEBKIT_AVAILABLE_OPTIONS_ISPUBLIC_${_name})

I also greatly prefer IS_PUBLIC to ISPUBLIC. I think we should fix INITIALVALUE as well.

> Source/cmake/WebKitFeatures.cmake:210
> +        # Print dots every other row, to improve readability of the features list.
>          if (_SHOULD_PRINT_POINTS)

Can we simply rename this variable to be clearer? Why not just call do _SHOULD_PRINT_POINTS to _SHOULD_PRINT_DOTS_FOR_READABILITY?

> CMakeLists.txt:183
> +# Print enabled features last for maximum visibility

Nit: Missing a period here.

[Michael Catanzaro]

Created attachment 250581 [details]
[CMake] Print sorted feature list at the very end

[Michael Catanzaro]

Sorry, the updated patch does not address your review comments, because I did not notice your review yet. :)

(In reply to comment #3)
> Maybe you could write "WebKit's custom allocator" instead of "malloc." I
> think "malloc" might be confusing because I think of that as the system
> allocator. :)

Yes, I meant to put bmalloc there, but actually I meant to do that in a separate patch. :p Hence the re-uploaded patch that does not include this change. I will do this in a separate style patch.

> > Source/cmake/WebKitFeatures.cmake:171
> > +    # Alphabetize.
> 
> Comments in WebKit should really be complete sentences. I'm not sure if a
> comment is really necessary here though, since list(SORT isn't much less
> obscure than "Alphabetize."

OK. It's probably not needed. I thought "sort how?" but it should be clear enough.

> > Source/cmake/WebKitFeatures.cmake:189
> > +        if (NOT _WEBKIT_AVAILABLE_OPTIONS_ISPUBLIC_${_name})
> 
> I also greatly prefer IS_PUBLIC to ISPUBLIC. I think we should fix
> INITIALVALUE as well.

OK. I will do this in the style patch (since this patch does not introduce the variable.)

> > Source/cmake/WebKitFeatures.cmake:210
> > +        # Print dots every other row, to improve readability of the features list.
> >          if (_SHOULD_PRINT_POINTS)
> 
> Can we simply rename this variable to be clearer? Why not just call do
> _SHOULD_PRINT_POINTS to _SHOULD_PRINT_DOTS_FOR_READABILITY?

Ummmmm... I don't think so, what was initially unclear to me was that the dots are intended to be printed on every other line. What I would think after seeing that name is "why should it sometimes not be readable?" Anyway, this comment should move to the style patch.

> > CMakeLists.txt:183
> > +# Print enabled features last for maximum visibility
> 
> Nit: Missing a period here.

OK

[Michael Catanzaro]

Created attachment 250582 [details]
[CMake] Print sorted feature list at the very end

[Martin Robinson]

(In reply to comment #5)

> > > Source/cmake/WebKitFeatures.cmake:210
> > > +        # Print dots every other row, to improve readability of the features list.
> > >          if (_SHOULD_PRINT_POINTS)
> > 
> > Can we simply rename this variable to be clearer? Why not just call do
> > _SHOULD_PRINT_POINTS to _SHOULD_PRINT_DOTS_FOR_READABILITY?
> 
> Ummmmm... I don't think so, what was initially unclear to me was that the
> dots are intended to be printed on every other line. What I would think
> after seeing that name is "why should it sometimes not be readable?" Anyway,
> this comment should move to the style patch.


I misread the original comment, but now that I understand it I agree with you.

[WebKit Commit Bot]

Comment on attachment 250582 [details]
[CMake] Print sorted feature list at the very end

Clearing flags on attachment: 250582

Committed r182663: <http://trac.webkit.org/changeset/182663>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.