On Fri, 20 Dec 2024 22:32:00 GMT, Mandy Chung <mch...@openjdk.org> wrote:
>> Henry Jen has updated the pull request with a new target base due to a merge >> or a rebase. The incremental webrev excludes the unrelated changes brought >> in by the merge/rebase. The pull request contains 16 additional commits >> since the last revision: >> >> - Merge remote-tracking branch 'openjdk/master' into JDK-8321413 >> - Javadoc and some minor refactoring >> - Move up Snippet setup as a builder >> - Merge remote-tracking branch 'openjdk/master' into JDK-8321413 >> - Minor cleanup >> - Rename based on feedback to emphasis building a snippet for loading a >> reference >> - Fix typo and comment to match latest implementation >> - Fix regression failed to setup helper methods properly >> - Separate out ModuleDescriptorBuilder and use LoadableArray for paging >> - Merge remote-tracking branch 'openjdk/master' into JDK-8321413 >> - ... and 6 more: https://git.openjdk.org/jdk/compare/3754332f...d5a93295 > > src/jdk.jlink/share/classes/jdk/tools/jlink/internal/Snippets.java line 227: > >> 225: */ >> 226: public CollectionSnippetBuilder enablePagination(String >> methodNamePrefix, int pageSize) { >> 227: return enablePagination(methodNamePrefix, pageSize, >> Math.min(pageSize, DEFAULT_THRESHOLD)); > > shouldn't it use the current `activatePagingThreshold` value? Otherwise, > this method will override the threshold being set to a non-default value. > > Same comment for `enablePagination(String methodNamePrefix)` I think about this, and it depends on how you look at the API. The idea is that those three parameters are essential to enable pagination from disabled state. A default value is used when user is to enable pagination not specifying an explicit value, so a clear default is adapted rather than lingering with current setting. Otherwise, user can use property methods to change page size or threshold. I like the idea of use methodNamePrefix as the enable/disable check. However, trying to explain the value of page size and threshold is current value, which would be default value without setting before is a little bit more complex, more so if trying to explain choosing the smaller value of page size and default threshold where we honestly have no idea if user have set a specific value or not. I switched default to disable in the latest commit. And if it helps, I can remove those two enablePagination methods using default values. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/21022#discussion_r1895312446
