src: introduce Constant class #303
Conversation
Codecov Report
@@ Coverage Diff @@
## master #303 +/- ##
=========================================
- Coverage 79.02% 77.6% -1.42%
=========================================
Files 33 33
Lines 4247 4260 +13
=========================================
- Hits 3356 3306 -50
- Misses 891 954 +63
Continue to review full report at Codecov.
|
|
I'll update the commit message later, but the main reason I started to work on this was to reduce noise from LookupConstant and LoadConstant debug messages. We don't want to see a message if a constant wasn't loaded but the replacement constant was (we were seeing two messages for the same constant in this case). The reduced noise from this PR helped me a lot while working on #255 (PR for that will come after this one lands). |
mmarchini
force-pushed
the
constant-class
branch
3 times, most recently
from
b324582 to
d819d66
Compare
|
cc @nodejs/llnode |
mmarchini
added
author ready
|
Ping @nodejs/llnode. This is the main blocker for the patches I have to fix inspection of Node.js v12 core dumps/processes. (I can rewrite those patches in the old way if this |
|
Ping @nodejs/llnode @nodejs/diagnostics @nodejs/post-mortem |
|
Ah, yes, that makes a lot more sense. I'll update the PR once I get a chance (probably later this week). Thank you for the suggestion :) |
As before, I kept the original LoadRawConstant and LoadConstant signatures unchanged to avoid a major refactor of |
mmarchini
force-pushed
the
constant-class
branch
from
6a44355 to
15912d9
Compare
Same idea as CheckedType: instead of relying on the default value for
constants to determine if they were loaded correctly or not (default
value usually was -1), have a class which will have information about
the loaded constant. This can help us:
- Check if a constant was properly loaded before using it (otherwise
we'd get into undefined behavior, and there are a lot of places
where this happens.
- Check if a constant value was loaded or if we're looking at the
default value.
- Explicitly define a constant as optional (constants which are not
relevant except for specific V8 versions).
This will help us improve reliability and debugability of llnode.
mmarchini
force-pushed
the
constant-class
branch
from
15912d9 to
ae4e13e
Compare
Same idea as CheckedType: instead of relying on the default value for
constants to determine if they were loaded correctly or not (default
value usually was -1), have a class which will have information about
the loaded constant. This can help us:
- Check if a constant was properly loaded before using it (otherwise
we'd get into undefined behavior, and there are a lot of places
where this happens.
- Check if a constant value was loaded or if we're looking at the
default value.
- Explicitly define a constant as optional (constants which are not
relevant except for specific V8 versions).
This will help us improve reliability and debugability of llnode.
PR-URL: #303
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
|
Landed in 2afd59e |
Same idea as CheckedType: instead of relying on the default value for
constants to determine if they were loaded correctly or not (default
value usually was -1), have a class which will have information about
the loaded constant. This can help us:
we'd get into undefined behavior, and there are a lot of places
where this happens.
default value.
relevant except for specific V8 versions).
This will help us improve reliability and debugability of llnode.