[Webkit-unassigned] [Bug 27980] Give an ability to WebKit to free statically allocated pointers before quit

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=27980


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #34058|review?                     |review-
               Flag|                            |




--- Comment #8 from Darin Adler <darin at apple.com>  2009-08-04 11:40:01 PDT ---
(From update of attachment 34058)
I think it's a good idea to make our globals more consistent.

But probably not such a great idea to start what could be a massive project
without a good idea how we'll keep test this and keep it working as each global
is added.

Is the goal here to make memory leak debugging tools work better? Or is there
value beyond that?

Could you write a document about how to use StaticPtr correctly? I'd like to
review that to get an idea how we would make sure we were using it properly on
various platforms. Would we consider it a coding style violation to make a
global that doesn't use StaticPtr?

As part of this are you proposing that DEFINE_STATIC_LOCAL be the only legal
way to make global variables?

I do not want to sign up everyone on the project to write a lot of code to
deallocate every last object if this code isn't actually useful to someone on
one of the ports. I need to hear more justification about why to take on all
this additional complexity. Making all the global data structures practical to
delete is an enormous task that this patch barely scratches the surface of.

An excellent economy in the project at the moment is that we don't try to write
all the deallocation code for all the global objects.

>      wtf/RefCountedLeakCounter.cpp \
>      wtf/TypeTraits.cpp \
> +    wtf/StaticPtr.cpp \
>      wtf/unicode/CollatorDefault.cpp \

I believe this is supposed to be in alphabetic order, so it should come before
TypeTraits.cpp.

> +#include "config.h"
> +
> +#include "StaticPtr.h"

Extra blank line here.

> +    StaticPtrBase() {
> +#if ENABLE(FREE_STATIC_PTRS)
> +        m_next = head();
> +        m_head = this;
> +#endif
> +    };

Extra semicolon here.

I think this would might read better if the inline implementation was outside
the class definition since it has an #if in it. There's no reason we have to
have the ifdef in here. But a better alternative would be to just leave out the
constructor definition entirely when FREE_STATIC_PTRS is not enabled. If there
are no constructor definitions then the class automatically will have an empty
default constructor which is what we want.

> +    StaticPtr(PtrType ptr) : StaticPtrBase(), m_ptr(ptr) { };
> +    StaticPtr() : StaticPtrBase(), m_ptr(0) { };
> +    ~StaticPtr() { };

No need to explicitly initialize StaticPtrBase since there are no constructor
arguments.

Extra semicolons here.

No need to explicitly declare the destructor -- this is the same as the
compiler-generated constructor and it's much better style to not declare it.

> +    PtrType operator=(PtrType);

This seems like bad style. Since values stored in this pointer will be freed, I
think we want this to use some explicit style like OwnPtr rather than plain
assignment.

> +    ValueType& operator=(ValueType&);

This seems quite strange. Why do we need to allow assignment to the pointer of
a non-const pointer of value type? This seems very dangerous, since the value
type will later be destroyed.

> +    PtrType operator&();

> +    PtrType operator&() const;

Do we need this operator? What coding style is going to require this?

> +    // Condition operators
> +    bool operator!();

Should be const.

Please look at OwnPtr for some ideas on how to make this class work. This
should be much closer to OwnPtr which has exactly the same semantic.

> +protected:

I believe these members should be private, not protected.

> +#if !PLATFORM(QT)
>      delete data;
> +#endif

This seems clearly wrong. Did you mean this to be a FREE_STATIC_PTRS ifdef?

> +    StaticGlobalData() : WTF::StaticPtr<JSGlobalData>() { };

This constructor is entirely unnecessary.

> +protected:

Should be private, not protected.

> +#if ENABLE(FREE_STATIC_PTRS)
> +    virtual void free() {
> +        m_ptr->heap.destroy();
> +        delete m_ptr;
> +    }
> +#endif

Incorrect brace style here.

> +ALWAYS_INLINE JSGlobalData* StaticGlobalData::operator=(JSGlobalData* value)
> +{
> +    m_ptr = value;
> +}

Seems poor to use plain assignment of a raw pointer for an operation that
transfers ownership.

> -    return globalData;
> +    return &globalData;

This line of code seems wrong. You want a JSGlobalData*, not a
StaticGlobalData*. This is a poor use of operator overloading.

review- for now based on the specific problems with the patch, but I do also
have project-direction concerns here, although I am not using reviewer power to
try to enforce those.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.