Closed mmarchini closed 4 years ago
Merging #303 into master will decrease coverage by
1.41%
. The diff coverage is70.58%
.
@@ 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
Impacted Files | Coverage Δ | |
---|---|---|
src/llv8.h | 80.95% <ø> (-2.39%) |
:arrow_down: |
src/llv8-constants.h | 98.48% <ø> (-1.52%) |
:arrow_down: |
src/error.h | 75% <0%> (-10.72%) |
:arrow_down: |
src/llv8-constants.cc | 82.05% <100%> (-1.02%) |
:arrow_down: |
src/llv8-inl.h | 86.66% <100%> (-6.25%) |
:arrow_down: |
src/constants.cc | 72.72% <65.78%> (-8.63%) |
:arrow_down: |
src/constants.h | 77.77% <83.33%> (+11.11%) |
:arrow_up: |
src/llv8.cc | 70.22% <0%> (-2.75%) |
:arrow_down: |
... and 3 more |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update ec01604...ae4e13e. Read the comment docs.
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).
cc @nodejs/llnode
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 Constant
class is not something we want, but from experience the code looks cleaner and easier to debug with this patch)
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 :)
std::pair<int64_t, bool>
bool loaded_
and bool valid_
Constants
to prevent misuse of the class (it should only be used to load constants).As before, I kept the original LoadRawConstant and LoadConstant signatures unchanged to avoid a major refactor of llv8-constants.cc
in a single PR.
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:
This will help us improve reliability and debugability of llnode.