1047564 - Tracelogger: cleanups

Status: RESOLVED FIXED

Description

[Hannes Verschore [:h4writer]]

Like discussed on IRC some name changes.

Comment 1

[Hannes Verschore [:h4writer]]

Created attachment 8466374 [details] [diff] [review]
cleanups

Comment 2

[Benjamin Bouvier [:bbouvier]]

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())

Comment 3

[Hannes Verschore [:h4writer]]

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.

Comment 4

[Hannes Verschore [:h4writer]]

*couldn't

Comment 5

[Benjamin Bouvier [:bbouvier]]

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?

Comment 6

[Hannes Verschore [:h4writer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/42312104737a

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/42312104737a