Tracelogger: cleanups

RESOLVED FIXED in mozilla34

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: h4writer, Assigned: h4writer)

Tracking

unspecified
mozilla34
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

4 years ago
Like discussed on IRC some name changes.
(Assignee)

Comment 1

4 years ago
Created attachment 8466374 [details] [diff] [review]
cleanups
Assignee: nobody → hv1989
Attachment #8466374 - Flags: review?(benj)
Comment on attachment 8466374 [details] [diff] [review]
cleanups

Review of attachment 8466374 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for doing that! I have an idea below...

::: js/src/vm/TraceLogging.cpp
@@ +259,5 @@
>          return true;
>      }
>  
>      JS_ASSERT(enabled == 1);
> +    while (stack.lastEntryId() > 0)

How about adding in ContinuousSpace:

bool empty() {
  return size_ == 0;
}

and test here (and in some other places)

while (!empty())
Attachment #8466374 - Flags: review?(benj) → review+
(Assignee)

Comment 3

4 years ago
Created attachment 8466411 [details] [diff] [review]
cleanups part2

Nice idea. What about this? (I'll push both merged offcourse)
I could use the empty() where you said. Because atm we never pop the root node.
Attachment #8466411 - Flags: review?(benj)
(Assignee)

Comment 4

4 years ago
*couldn't
Comment on attachment 8466411 [details] [diff] [review]
cleanups part2

Review of attachment 8466411 [details] [diff] [review]:
-----------------------------------------------------------------

Cool, thanks! One more idea, if you agree: as the stack.size() == or > 1 (i.e. root stack entry) check is kind of weird for new comers, how about 

A) either making an inline bool Tracelogging::hasNonRootStackEntries() { return stack.size() > 1 ; }
B) or add a comment above StackEntry or in Tracelogger::init(), or somewhere it should belong (above all stack.size() == 1 or > 1 would be unfortunate though), to explain that we need the root stack entry?
Attachment #8466411 - Flags: review?(benj) → review+
https://hg.mozilla.org/mozilla-central/rev/42312104737a
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.