WeakPtrFactory::GetMutableWeakPtr() proposal

[Alan Cutter]

Currently it is easy to accidentally acquire a mutable this pointer in a const method if you own a WeakPtrFactory. This is due to its GetWeakPtr() const interface dispensing a WeakPtr<T>. I've fixed up one accidental logical const violation I came across and I suspect there may be dozens of others in the codebase.

Given the guidance on using const in Chromium I think we should change the WeakPtrFactory interface from:

  WeakPtr<T> GetWeakPtr() const;

to:

  WeakPtr<T> GetWeakPtr();

  WeakPtr<const T> GetWeakPtr() const;

  WeakPtr<T> GetMutableWeakPtr() const;

Using GetMutableWeakPtr() is still valid in certain scenarios e.g. your WeakPtrFactory is const while your this context is mutable. By giving it an explicit non-const name this should prevent further accidental const violations slipping through in future.

[David Benjamin]

This sounds like a good change to me!

What's the reason for making the WeakPtrFactory const? I guess it prevents you from calling InvalidateWeakPtrs(), but I dunno how important that is. If we removed the const from those classes, would we still need GetMutableWeakPtr? (Callers that need to get non-const WeakPtr from const this can always const_cast first, which matches what you'd have to do for a raw pointer anyway.)

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/086167b4-7428-48f6-a1a4-bace8fc669den%40chromium.org.

[Alan Cutter]

I suspect we don't want to allow const_cast usage in general and specific cases like these can be "blessed" as explicit exceptions.

It would be nice to not have GetMutableWeakPtr() but I think its usage is prevalent enough that it'll be too much work to refactor those uses out of the codebase.

[dan...@chromium.org]

+1 to better const correctness. Explicit internal mutability in a library type like this makes sense to me. Would you be undertaking this change?

If I was to more closely match the naming patterns of well-established const-correct libraries, I'd name it GetWeakPtrMut(). 😇

On Thu, Sep 1, 2022 at 8:24 PM Alan Cutter <alanc...@chromium.org> wrote:

--

[Alan Cutter]

On Wednesday, 7 September 2022 at 12:01:07 am UTC+10 danakj wrote:

+1 to better const correctness. Explicit internal mutability in a library type like this makes sense to me. Would you be undertaking this change?

I'll pursue seeing how much work is involved in my spare cycles. Just wanted to check for high level agreement before diving into the task.

 

If I was to more closely match the naming patterns of well-established const-correct libraries, I'd name it GetWeakPtrMut(). 😇

SGTM. What are those libraries? This pattern is new to me.

[dan...@chromium.org]

On Wed, Sep 7, 2022 at 12:16 AM Alan Cutter <alanc...@chromium.org> wrote:

On Wednesday, 7 September 2022 at 12:01:07 am UTC+10 danakj wrote:

+1 to better const correctness. Explicit internal mutability in a library type like this makes sense to me. Would you be undertaking this change?

I'll pursue seeing how much work is involved in my spare cycles. Just wanted to check for high level agreement before diving into the task.

 

If I was to more closely match the naming patterns of well-established const-correct libraries, I'd name it GetWeakPtrMut(). 😇

SGTM. What are those libraries? This pattern is new to me.

One such example:

https://doc.rust-lang.org/std/collections/struct.BTreeMap.html#method.iter

https://doc.rust-lang.org/std/collections/struct.BTreeMap.html#method.iter_mut

[Alan Cutter]

On Thursday, 8 September 2022 at 12:15:34 am UTC+10 danakj wrote:

On Wed, Sep 7, 2022 at 12:16 AM Alan Cutter <alanc...@chromium.org> wrote:

On Wednesday, 7 September 2022 at 12:01:07 am UTC+10 danakj wrote:

+1 to better const correctness. Explicit internal mutability in a library type like this makes sense to me. Would you be undertaking this change?

I'll pursue seeing how much work is involved in my spare cycles. Just wanted to check for high level agreement before diving into the task.

 

If I was to more closely match the naming patterns of well-established const-correct libraries, I'd name it GetWeakPtrMut(). 😇

SGTM. What are those libraries? This pattern is new to me.

One such example:

https://doc.rust-lang.org/std/collections/struct.BTreeMap.html#method.iter

https://doc.rust-lang.org/std/collections/struct.BTreeMap.html#method.iter_mut

Hmm, I'm a bit less convinced by Rust libraries where mut is a language level keyword. Feels like language syntax cross-contamination.

[Alan Cutter]

FYI: This change has been completed: CL

GetWeakPtr() const now returns WeakPtr<const T> instead of WeakPtr<T>.

[dan...@chromium.org]

Thank you Alan, this is a great contribution to reducing global knowledge / increasing our ability to reason locally by making mutation paths more clear and less viral.