RFR: 8264565: Templatize num_arguments() functions of DCmd subclasses

[Ioi Lam-2]

We have many version of `num_arguments()` for the `DCmd` subclasses. They all have identical structure. We should templatize them to reduce duplicated code and avoid cut-and-paste errors.

It's still possible to write a customized `num_arguments()` function (although none of the existing cases needs to do that). The rules are described here:

class DCmd : public ResourceObj {
  ...
  // num_arguments() is used by the DCmdFactoryImpl::get_num_arguments() template functions.
  // - For subclasses of DCmdWithParser, it's calculated by DCmdParser::num_arguments().
  // - Other subclasses of DCmd have zero arguments by default. You can change this
  //   by defining your own version of MyDCmd::num_arguments().
  static int num_arguments()        { return 0; }

-------------

Commit messages:
 - removed remaining boilerplate num_arguments() functions
 - DebugOnCmdStartDCmd does not need to subclass from DCmdWithParser
 - 8264565: Templatize num_arguments() functions of DCmd subclasses

Changes: https://git.openjdk.java.net/jdk/pull/3312/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3312&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8264565
  Stats: 277 lines in 8 files changed: 27 ins; 241 del; 9 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3312.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3312/head:pull/3312

PR: https://git.openjdk.java.net/jdk/pull/3312

[Ioi Lam-2]

On Thu, 1 Apr 2021 17:46:22 GMT, Ioi Lam <[hidden email]> wrote:

> We have many version of `num_arguments()` for the `DCmd` subclasses. They all have identical structure. We should templatize them to reduce duplicated code and avoid cut-and-paste errors.
>
> It's still possible to write a customized `num_arguments()` function (although none of the existing cases needs to do that). The rules are described here:
>
> class DCmd : public ResourceObj {
>   ...
>   // num_arguments() is used by the DCmdFactoryImpl::get_num_arguments() template functions.
>   // - For subclasses of DCmdWithParser, it's calculated by DCmdParser::num_arguments().
>   // - Other subclasses of DCmd have zero arguments by default. You can change this
>   //   by defining your own version of MyDCmd::num_arguments().
>   static int num_arguments()        { return 0; }

For verification, I added code in an earlier commit (hence removed) to print out the `num_arguments()` of each `DCmd`. The results are identical with and without this patch.

https://github.com/openjdk/jdk/blob/25920cdd8296901bcd64d5cf277a616bc90bea90/src/hotspot/share/services/diagnosticFramework.cpp#L521-L523

-------------

PR: https://git.openjdk.java.net/jdk/pull/3312

[Coleen Phillimore-3]

On Thu, 1 Apr 2021 17:46:22 GMT, Ioi Lam <[hidden email]> wrote:

> We have many version of `num_arguments()` for the `DCmd` subclasses. They all have identical structure. We should templatize them to reduce duplicated code and avoid cut-and-paste errors.
>
> It's still possible to write a customized `num_arguments()` function (although none of the existing cases needs to do that). The rules are described here:
>
> class DCmd : public ResourceObj {
>   ...
>   // num_arguments() is used by the DCmdFactoryImpl::get_num_arguments() template functions.
>   // - For subclasses of DCmdWithParser, it's calculated by DCmdParser::num_arguments().
>   // - Other subclasses of DCmd have zero arguments by default. You can change this
>   //   by defining your own version of MyDCmd::num_arguments().
>   static int num_arguments()        { return 0; }

Wow cool and nice.

-------------

Marked as reviewed by coleenp (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3312

[Ioi Lam]

Hi, could I have one more review for this?

Thanks
- Ioi

On 4/1/21 11:11 AM, Ioi Lam wrote:

> We have many version of `num_arguments()` for the `DCmd` subclasses. They all have identical structure. We should templatize them to reduce duplicated code and avoid cut-and-paste errors.
>
> It's still possible to write a customized `num_arguments()` function (although none of the existing cases needs to do that). The rules are described here:
>
> class DCmd : public ResourceObj {
>    ...
>    // num_arguments() is used by the DCmdFactoryImpl::get_num_arguments() template functions.
>    // - For subclasses of DCmdWithParser, it's calculated by DCmdParser::num_arguments().
>    // - Other subclasses of DCmd have zero arguments by default. You can change this
>    //   by defining your own version of MyDCmd::num_arguments().
>    static int num_arguments()        { return 0; }
>
> -------------
>
> Commit messages:
>   - removed remaining boilerplate num_arguments() functions
>   - DebugOnCmdStartDCmd does not need to subclass from DCmdWithParser
>   - 8264565: Templatize num_arguments() functions of DCmd subclasses
>
> Changes: https://git.openjdk.java.net/jdk/pull/3312/files
>   Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3312&range=00
>    Issue: https://bugs.openjdk.java.net/browse/JDK-8264565
>    Stats: 277 lines in 8 files changed: 27 ins; 241 del; 9 mod
>    Patch: https://git.openjdk.java.net/jdk/pull/3312.diff
>    Fetch: git fetch https://git.openjdk.java.net/jdk pull/3312/head:pull/3312
>
> PR: https://git.openjdk.java.net/jdk/pull/3312

[David Holmes-2]

On Thu, 1 Apr 2021 17:46:22 GMT, Ioi Lam <[hidden email]> wrote:

> We have many version of `num_arguments()` for the `DCmd` subclasses. They all have identical structure. We should templatize them to reduce duplicated code and avoid cut-and-paste errors.
>
> It's still possible to write a customized `num_arguments()` function (although none of the existing cases needs to do that). The rules are described here:
>
> class DCmd : public ResourceObj {
>   ...
>   // num_arguments() is used by the DCmdFactoryImpl::get_num_arguments() template functions.
>   // - For subclasses of DCmdWithParser, it's calculated by DCmdParser::num_arguments().
>   // - Other subclasses of DCmd have zero arguments by default. You can change this
>   //   by defining your own version of MyDCmd::num_arguments().
>   static int num_arguments()        { return 0; }

Looks like a good cleanup!

Thanks,
David

-------------

Marked as reviewed by dholmes (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3312

[Ioi Lam-2]

> We have many version of `num_arguments()` for the `DCmd` subclasses. They all have identical structure. We should templatize them to reduce duplicated code and avoid cut-and-paste errors.
>
> It's still possible to write a customized `num_arguments()` function (although none of the existing cases needs to do that). The rules are described here:
>
> class DCmd : public ResourceObj {
>   ...
>   // num_arguments() is used by the DCmdFactoryImpl::get_num_arguments() template functions.
>   // - For subclasses of DCmdWithParser, it's calculated by DCmdParser::num_arguments().
>   // - Other subclasses of DCmd have zero arguments by default. You can change this
>   //   by defining your own version of MyDCmd::num_arguments().
>   static int num_arguments()        { return 0; }

Ioi Lam 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 four additional commits since the last revision:

 - Merge branch 'master' into 8264565-templatize-dcmd-num-arguments
 - removed remaining boilerplate num_arguments() functions
 - DebugOnCmdStartDCmd does not need to subclass from DCmdWithParser
 - 8264565: Templatize num_arguments() functions of DCmd subclasses

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3312/files
  - new: https://git.openjdk.java.net/jdk/pull/3312/files/ec38f55e..373b1c11

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3312&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3312&range=00-01

  Stats: 17290 lines in 172 files changed: 13403 ins; 2198 del; 1689 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3312.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3312/head:pull/3312

PR: https://git.openjdk.java.net/jdk/pull/3312

[Ioi Lam-2]

On Fri, 2 Apr 2021 16:46:50 GMT, Coleen Phillimore <[hidden email]> wrote:

>> Ioi Lam 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 four additional commits since the last revision:
>>
>>  - Merge branch 'master' into 8264565-templatize-dcmd-num-arguments
>>  - removed remaining boilerplate num_arguments() functions
>>  - DebugOnCmdStartDCmd does not need to subclass from DCmdWithParser
>>  - 8264565: Templatize num_arguments() functions of DCmd subclasses
>
> Wow cool and nice.

Thanks @coleenp and @dholmes-ora for the review.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3312

[Ioi Lam-2]

On Thu, 1 Apr 2021 17:46:22 GMT, Ioi Lam <[hidden email]> wrote:

> We have many version of `num_arguments()` for the `DCmd` subclasses. They all have identical structure. We should templatize them to reduce duplicated code and avoid cut-and-paste errors.
>
> It's still possible to write a customized `num_arguments()` function (although none of the existing cases needs to do that). The rules are described here:
>
> class DCmd : public ResourceObj {
>   ...
>   // num_arguments() is used by the DCmdFactoryImpl::get_num_arguments() template functions.
>   // - For subclasses of DCmdWithParser, it's calculated by DCmdParser::num_arguments().
>   // - Other subclasses of DCmd have zero arguments by default. You can change this
>   //   by defining your own version of MyDCmd::num_arguments().
>   static int num_arguments()        { return 0; }

This pull request has now been integrated.

Changeset: ff223530
Author:    Ioi Lam <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/ff223530
Stats:     277 lines in 8 files changed: 27 ins; 241 del; 9 mod

8264565: Templatize num_arguments() functions of DCmd subclasses

Reviewed-by: coleenp, dholmes

-------------

PR: https://git.openjdk.java.net/jdk/pull/3312