Closed Bug 956400 Opened 10 years ago Closed 3 years ago

Annotate SizeOf..cludingThis.* functions as MOZ_WARN_UNUSED_RESULT

Categories

(Core :: General, defect)

Product:
Core
Component:
General
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WONTFIX

People

(Reporter: ehsan.akhgari, Unassigned)

Details

WDYT about doing something like:

template<class T>
struct SizeOfMixin
{
  MOZ_WARN_UNUSED_RESULT size_t SizeOfIncludingThis(...) {
    return static_cast<T*>(this)->SizeOfIncludingThisImplementation(...);
  }

  MOZ_WARN_UNUSED_RESULT size_t SizeOfExcludingThis(...) {
    return static_cast<T*>(this)->SizeOfExcludingThisImplementation(...);
  }
};

class ThatMeasuresItsMemory : public SizeOfMixin<ThatMeasuresItsMemory> {
  ...
};

I admit that this is a little more fiddly that just slapping MOZ_WARN_UNUSED_RESULT on all the current implementations, but it has the benefit that future changes should just Do The Right Thing automatically.  (Assuming that the reviewers know patches should be using this mixin...)
Well...

(a) not every class implements both IncludingThis and ExcludingThis variants, and occasionally we have variations such as AddSizeOfIncludingThis() or SizeOfSomethingElse();

(b) | : public SizeOfMixin<Foo>| is longer than |MOZ_WARN_UNUSED_RESULT|, plus we'd need to append |Implementation| (or perhaps just |Impl|) to a lot of function names.

So I don't think it'll make the code more concise, nor will it reduce code churn.
Yeah.  I don't think we can avoid touching the actual functions here.
I just started looking at this. We have a *lot* of functions that need annotating; hundreds and hundreds of them. It's enough that I'm wondering if adding a few macros like this would be a good idea:

  #define MOZ_DECL_SIZE_OF_INCLUDING_THIS \
      MOZ_WARN_UNUSED_RESULT size_t \
      SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const;

  #define MOZ_DECL_SIZE_OF_EXCLUDING_THIS \
      MOZ_WARN_UNUSED_RESULT size_t \
      SizeOfExcludingThis(mozilla::MallocSizeOf aMallocSizeOf) const;

These would probably go in mfbt/MemoryReporting.h, which is where mozilla::MallocSizeOf is defined.

It would reduce code size considerably -- MOZ_WARN_UNUSED_RESULT is long enough that adding it causes most of these declarations to go from one line to two. Also, it's easy to forget the |const| at the end, and having macros like this would help avoid that.

Not all such functions would get a macro -- there are some odd cases that don't fit the basic patterns. But *many* functions do fit the basic patterns.

I guess a typedef might also work, but we have precent for this kind of thing with NS_DECL_ISUPPORTS and all that...
>   #define MOZ_DECL_SIZE_OF_INCLUDING_THIS \
>       MOZ_WARN_UNUSED_RESULT size_t \
>       SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const;

I'm partway through doing this. However, quite a few of these instances have |virtual| at the start, which works with this macro, but some also have |MOZ_OVERRIDE| at the end. So I wonder about doing this instead:

>   #define MOZ_DECL_SIZE_OF_INCLUDING_THIS \
>       MOZ_WARN_UNUSED_RESULT size_t \
>       SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const

So that you could then use it like this, for example:

  virtual MOZ_DECL_SIZE_OF_INCLUDING_THIS MOZ_OVERRIDE;

This means you'd always need to append the trailing ';', which is a bit different to the existing NS_DECL_/MOZ_DECL_ macros, but that doesn't seem too bad.

Nick
(In reply to comment #5)
> >   #define MOZ_DECL_SIZE_OF_INCLUDING_THIS \
> >       MOZ_WARN_UNUSED_RESULT size_t \
> >       SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const;
> 
> I'm partway through doing this. However, quite a few of these instances have
> |virtual| at the start, which works with this macro, but some also have
> |MOZ_OVERRIDE| at the end. So I wonder about doing this instead:
> 
> >   #define MOZ_DECL_SIZE_OF_INCLUDING_THIS \
> >       MOZ_WARN_UNUSED_RESULT size_t \
> >       SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const
> 
> So that you could then use it like this, for example:
> 
>   virtual MOZ_DECL_SIZE_OF_INCLUDING_THIS MOZ_OVERRIDE;

Whoa, this is pretty terrible.  Please don't do this.  I'd much rather have the code span two or more lines there.
What's terrible -- adding |virtual| and/or |MOZ_OVERRIDE|? If we don't allow that, we'd just have to fallback to not using the macro. Is that what you want?
(In reply to comment #7)
> What's terrible -- adding |virtual| and/or |MOZ_OVERRIDE|?

|virtual MOZ_DECL_SIZE_OF_INCLUDING_THIS MOZ_OVERRIDE;| doesn't look like C++ at all, and it violates our usual NS_DECL_FOO style.

> If we don't allow
> that, we'd just have to fallback to not using the macro. Is that what you want?

Yes, I prefer avoiding macros in that case.  Using them for the non-virtual methods is fine as far as I'm concerned but you may choose to not do that for consistency if you want.
Perhaps bug 1006667 would provide a decent middle-ground? We could annotate the helper SizeOf functions and hopefully switch most usage over to those.

As far as macros go, as a writer of far too many SizeOf functions, I'd prefer that MOZ_DECL_SIZE_OF_INCLUDING_THIS declare the function body. Now of course there's the argument that sometimes we combine Excluding with Including to be more concise, but we could just stop doing that, I don't think it would be any less concise now that we have these macros.

And then to side-step the virtual / virtual+override issue we could possibly have 2 extra versions:
    #define MOZ_DECL_VIRTUAL_SIZE_OF ...
    #define MOZ_DECL_OVERRIDE_SIZE_OF ...

Although I guess there's an argument that this is just getting pretty tedious and confusing. A further question is what would things look like for cases where we want to just inline the definition in the class/struct declaration? Think:

class A {
  ...
private:
  class InternalThing {
     SizeOfExcludingThis { ... }
  };
};
These are all reasonable ideas and questions. The exact best way forward isn't clear. Feel free to take this over if you want; I certainly haven't gotten anywhere with it.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.