Closed Bug 1083020 Opened 5 years ago Closed 5 years ago

Add a (dummy) stats method to all LibHandles

Categories

(Core :: mozglue, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(1 file)

This is the exact opposite of bug 1082529, because that bug requires bug 1082529 and bug 1082527 first in order to happen.
While here, avoid doing anything if debug logging is disabled.
Attachment #8505267 - Flags: review?(nfroyd)
Comment on attachment 8505267 [details] [diff] [review]
Add a (dummy) stats method to all LibHandles

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

How about we do:

class LibHandle {
protected:
  // or statsGuts, or whatever.
  virtual void printStats(const char* when) { };

public:
  void stats(const char* when) {
    if (MOZ_LIKELY(!Logging::IsVerbose())) {
      return;
    }
    printStats(when);
  }
};

That way we don't have to think about making sure all the stats() methods DTRT.  WDYT?
Attachment #8505267 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd (:froydnj) from comment #2)
> That way we don't have to think about making sure all the stats() methods
> DTRT.  WDYT?

Unfortunately, the compiler doesn't skip the loop in ElfLoader::stats without the check before it. The check in MappableSeekableZStream::stats can be removed, actually, which just leaves the one in CustomElf::Load as extra weight.
(In reply to Mike Hommey [:glandium] from comment #3)
> (In reply to Nathan Froyd (:froydnj) from comment #2)
> > That way we don't have to think about making sure all the stats() methods
> > DTRT.  WDYT?
> 
> Unfortunately, the compiler doesn't skip the loop in ElfLoader::stats
> without the check before it.

Wouldn't the loop get skipped because that method never gets called?

Either way, doesn't make much difference to me.  OK to land as is.
Oh, maybe the logging is compile-time determined, not runtime.  In that case, I now understand.
https://hg.mozilla.org/mozilla-central/rev/1f5fdd080b46
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.