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.
Created attachment 250500 [details] [CMake] Print sorted feature list at the very end
Created attachment 250578 [details] [CMake] Print sorted feature list at the very end
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.
Created attachment 250581 [details] [CMake] Print sorted feature list at the very end
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
Created attachment 250582 [details] [CMake] Print sorted feature list at the very end
(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.
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>
All reviewed patches have been landed. Closing bug.