lttng: updated flags for gc tracing#3388
Uh oh!
There was an error while loading. Please reload this page.
Conversation
mscdex added the post-mortem 
Fishrock123 commented Oct 16, 2015
cc @nodejs/lts |
rvagg commented Oct 16, 2015
@thekemkid do you know how long this has been broken? it'd be nice if we could get some lttng folks using releases more often so this stuff was kept in shape. |
GlenTiki commented Oct 16, 2015
@rvagg it looks like it was working in the v4.1.2 proposal branch. I'm planning on doing some more work on lttng/adding more lttng tracepoints so I'll keep maintaining it. :) |
src/node_lttng_provider.h Outdated
There was a problem hiding this comment.
Long lines, please keep them <= 80 columns.
There was a problem hiding this comment.
Would it be okay if I used a switch here instead?
Heres what I'm thinking:
switch (flags){case v8::GCCallbackFlags::kNoGCCallbackFlags: flagsStr = "kNoGCCallbackFlags"; break; case v8::GCCallbackFlags::kGCCallbackFlagConstructRetainedObjectInfos; flagsStr = "kGCCallbackFlagConstructRetainedObjectInfos"; break; case v8::GCCallbackFlags::kGCCallbackFlagForced: flagsStr = "kGCCallbackFlagForced"; break; case v8::GCCallbackFlags::kGCCallbackFlagSynchronousPhantomCallbackProcessing: flagsStr = "kGCCallbackFlagSynchronousPhantomCallbackProcessing"; break; default: flagsStr = "Unrecognised GCCallbackFlag"; break}There was a problem hiding this comment.
That looks a little awkward. You could add a using F = v8::GCCallbackFlags a few lines up so you can shorten the names but I'd probably use an x-macro to generate the clauses. That lets you drop the duplication of the enum and the string as well. You can find an example of an x-macro in src/node_v8.cc, grep for HEAP_STATISTICS_PROPERTIES.
There was a problem hiding this comment.
Done! I also shortened the GCType if statement with an x-macro too.
bnoordhuis commented Oct 16, 2015
The breakage is because of the recent upgrade to V8 4.6 in master. |
GlenTiki commented Oct 17, 2015
Okay, I think this is ready to be reviewed again :) Let me know if I need to squash the commits. |
GlenTiki commented Oct 20, 2015
could I get another review? @nodejs/lts |
PR-URL: #3388 Reviewed-By: Ben Noordhuis <[email protected]>
bnoordhuis commented Oct 20, 2015
Landed in 9adc6a6, thanks. Can you make sure next time that |
MylesBorins commented Oct 20, 2015
Should we add an LTS tag here? |
Fishrock123 commented Oct 20, 2015
No need. :) |
GlenTiki commented Oct 20, 2015
@bnoordhuis will do, thanks :) |
PR-URL: #3388 Reviewed-By: Ben Noordhuis <[email protected]>
MylesBorins commented Dec 29, 2015
MylesBorins commented Jan 6, 2016
7 participants
This fixes some build errors I was getting while trying to build --with-lttng. :)