src: use struct as arguments to node::Assert #25869
Conversation
nodejs-github-bot
added
the
c++
| @@ -120,9 +126,10 @@ void DumpBacktrace(FILE* fp); | |||
| #define CHECK(expr) \ | |||
| do { \ | |||
There was a problem hiding this comment.
If you're here, would you consider de-crufting this? i.e.:
if (!(expr)) { \
node::Assert({ __FILE__, __LINE__, #expr, PRETTY_FUNCTION_NAME }); \
} \There was a problem hiding this comment.
@refack At least locally (gcc-7 on Ubuntu 18.04), that would undo the purpose of this (what the comment is referring to): It would put the object construction in-line into the code instead of ending up as a simple pointer load from the read-only data section.
There was a problem hiding this comment.
So how about:
if (!(expr)) { \
static constexpr args{ __FILE__, __LINE__, #expr, PRETTY_FUNCTION_NAME } \
node::Assert(args); \
}There was a problem hiding this comment.
@refack You are talking about const → constexpr, right? That has the same result for me, and I don’t see it as being clearer about the intention here (having an explicit, hardcoded object in the read-only data section).
I’ll make the comment more explicit about what the idea here is.
This just makes the code a bit more obvious (subjectively).
addaleax
added
the
author ready
|
Landed in 30545a5 |
This just makes the code a bit more obvious (subjectively). PR-URL: #25869 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
This just makes the code a bit more obvious (subjectively). PR-URL: #25869 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
This just makes the code a bit more obvious (subjectively).
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes