src: adjust THP sysfs config token retrieval and file closure

[jayaddison]

This change is a refactor that builds upon and extends #37065. The pull request title might not be ideal; I'm open to other suggestions.

It includes the modifications described in #37065 (comment):

• A std::ifstream object's close member function will automatically be called when the object is destroyed; this may enable some conditional-clause rewrites
• Since we are only reading a fixed number of tokens, we could sequentially read these using a single variable

Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license
indicated in the file; or

(b) The contribution is based upon previous work that, to the best
of my knowledge, is covered under an appropriate open source
license and I have the right under that license to submit that
work with modifications, whether created in whole or in part
by me, under the same open source license (unless I am
permitted to submit under a different license), as indicated
in the file; or

(c) The contribution was provided directly to me by some other
person who certified (a), (b) or (c) and I have not modified
it.

(d) I understand and agree that this project and the contribution
are public and that a record of the contribution (including all
personal information I submit with it, including my sign-off) is
maintained indefinitely and may be redistributed consistent with
this project or the open source license(s) involved.

[jayaddison]

PS: Apologies for not properly handling column width limits in git commit messages (again, despite mentioning that I would). It seemed preferable to retain existing commit history since this branch had already been mentioned in a previous comment.

[RaisinTen]

@jayaddison could you please get rid of the merge commit? It tends to break our tooling. :/

[jayaddison]

Thanks @RaisinTen. Given that a fast-forward merge from the master branch doesn't appear to be an option from the original proposed changes in this pull request, I think I'll incorporate your changes, which look good, then rebase and force-push as long as all's well after continuous integration.

[jayaddison]

@RaisinTen Are you able to convert this into a 'Draft' pull request until I apply a few of the suggestions here? (I don't appear to have permissions, and I'd like to avoid unnecessarily running CI jobs while applying a few changes)

[jayaddison]

Thanks, @aduh95!

[jayaddison]

Closing and re-opening the pull request momentarily in order to run the full continuous integration suite.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/35928/

[RaisinTen]

Still LGTM.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/35976/

[jayaddison]

Thanks @RaisinTen - there's at least one more change that I'm considering adding, which is whether to move the constant comparison values (false, [always], [madvise]) to the left-hand side of the comparison operators. Especially in the case of the function call, this really shouldn't be necessary, but I worry a little that this could be extremely frequently run code, so if there are small changes that can reduce risk, those may be worthwhile.

The part of this code that still leaves me with some concern in this area is the use of the (very overloaded) >> operator.

[RaisinTen]

Sure, go ahead! :)
What do you intend to use in place of the >> operator?

[jayaddison]

I don't yet have a replacement in mind; the operator looks correct and safe in the current context, but any operator that has a non-trivial number of different behaviours based on surrounding datatypes seems worth being wary of.

The surrounding code could change in future for a variety of reasons, and in the absence of unit test coverage, finding ways to make it more likely that the code would fail to compile if it became incorrect would be useful.

The tradeoff is that changing the code itself has to be done carefully to ensure that it remains correct, and that requires equally careful review and attention. That is a counter-balancing effect that makes me slightly reluctant to make any significant modifications.

[jayaddison]

I'll pause on any further changes here now, pending continued review.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/35997/

[jayaddison]

This may be over-analysis, but on second thoughts, I don't think that moving the constant expression to the left at

node/src/large_pages/node_large_page.cc

Line 265 in 0b3d492

if (false == config_stream.good()) {

is a worthwhile tradeoff against the readability of the code.

The . member access operator has higher precedence than the logical not operator (ref) so it should be clear to readers familiar with C++ that no operator overloading is applied to the boolean result of good.

tl;dr - if (!config_stream.good()) seems better than if (false == config_stream.good()), and I'll apply that change in a moment

[RaisinTen]

I agree, it looks clearer to me too.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/36040/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/36139/

[aduh95]

Forced-push to remove the merge commits. I wasn't sure what commit message to use, can you confirm the one I've picked is OK?

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/36147/

[jayaddison]

Thanks @aduh95 - yep, that commit message looks good to me 👍

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/36150/

[jayaddison]

@aduh95 Actually, perhaps it's a little pedantic, but maybe it's better to use the word 'adjust' rather than 'refactor' in the commit message since there is a small behaviour change here. I'm just thinking of any users who might be reading through a changelog/commit log. I'll apply that commit message update and re-force-push momentarily.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/36153/

[aduh95]

@jayaddison alright, thanks! FYI, Node.js collaborators can amend the commit message when landing, so we can avoid having to re-spawn a CI job and land the PR quicker. Anyways, CI is running, this can land once it's green.

[jayaddison]

@aduh95 Ok, thanks; still learning the ropes a bit here!

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/36157/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/36166/

[aduh95]

Landed in 9b32762

[jayaddison]

Thanks @aduh95!