crypto: move field initialization to class

[theholla]

Initialize fields of ClientHelloParser in class rather than in constructor.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

[addaleax]

I think you also need/want to add default initializers here, for the 5 that are later applied to ClientHello directly?

[theholla]

I've updated the PR and moved the default initializers from ClientHello to ClientHelloParser, which I believe is what was needed in the first place.

Thanks for catching that. I'm not very familiar with C++ and appreciate the keen eye!

[addaleax]

@theholla Just for context – I don’t think there was anything wrong with adding the initializers to ClientHello, as long as ClientHelloParser has all initializers that were previously in its own constructor :)

[theholla]

That's good to know, thanks! While you're here, I wonder if you see any issues with the two fields that are listed as one type in one class, and another in the next? Both servername_size and ocsp_request are listed as different types between ClientHello and ClientHelloParser.

class ClientHelloParser {
  class ClientHello {
    private:
      uint8_t servername_size_;
      int ocsp_request_;
  };
  private:
    uint16_t servername_size_ = 0;
    uint8_t ocsp_request_ = 0;
}

This predated the PR but stood out to me as a possible issue.

[addaleax]

Puh … that’s a good question.

For ocsp_request_, I searched for instances of that in the source code, and it seems like the only values are 0 and 1 – I think you could change the type in one so it matches the other, if you like. (I could totally be wrong about this being a “boolean“, though…)

For servername_size_, I’m not sure. It looks to me like we wouldn’t do anything wrong by making both fields uint16_ts, and remove the static_cast when converting …

Either way, this might be better answered by @nodejs/crypto experts?

[addaleax]

Thanks!

[addaleax]

CI: https://ci.nodejs.org/job/node-test-pull-request/17779/ [scheduled]

[ChALkeR]

LGTM.

Should be squashed (could be done at landing, I presume).

As a side note, it's interesting that tls_ticket_size_ = 0 appears unused, as it's immediately set to -1 in Reset() which is called directly from the constructor. But I would suggest not to touch that in this PR for now, this PR looks good.

[Trott]

Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/1​77​98

[Trott]

Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/17820/

[Trott]

Collaborators, please 👍 here to approve fast-tracking.

[addaleax]

Landed in 98afca9

Thanks for the contribution! 🎉

(If you're interested in other possible contributions to Node.js but don't have a good idea of where to start looking, some ideas are posted at https://www.nodetodo.org/next-steps/.)