src: further simplify large pages code #32576
Conversation
* Clean up and co-locate explanation for what the code does to the top of the file. * Remove extraneous headers. * Remove the need for `OnScopeLeave` replacing it with a pair of labels `fail:` and `done:`. Re: nodejs#32570 Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
nodejs-github-bot
added
the
c++
gabrielschulhof
requested review from
bnoordhuis,
addaleax,
devnexen and
jasnell
| return -1; | ||
| status = -1; | ||
| done: | ||
| if (nmem != nullptr && nmem != MAP_FAILED && munmap(nmem, size) == -1) |
There was a problem hiding this comment.
nit: maybe possible to avoid code repetition here what do you think ?
There was a problem hiding this comment.
Oh, right, with a (nmem != nullptr && ... ) || (tmem != nullptr && ... ). @jasnell had suggested that previously.
There was a problem hiding this comment.
Well, actually, you wanna munmap them both, even if the first one fails. So, you may PrintSystemError() because the first munmap failed, because the second munmap failed, twice because they both failed, or not at all.
There was a problem hiding this comment.
AFAICT to avoid this code repetition we have to go all the way to #32570 which I feel is a bit too much.
There was a problem hiding this comment.
We could do __attribute__((cleanup("DeleteMmapPointer"))) for nmem and tmem but I'm not sure how portable that is and we would have two levels of indirection because we'd have to store nmem along with size in some structure which would be passed to the deleter. Same for tmem.
Again, it seems like too much complication for avoiding a single repetition.
|
I’d prefer #32570 here. |
of the file.
OnScopeLeavereplacing it with a pair of labelsfail:anddone:.Re: #32570
Signed-off-by: @gabrielschulhof
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesThis is an alternative to #32570.