1691913 - Better align nsBaseHashtable::Get and nsBaseHashtable::GetOrInsert

Status: RESOLVED FIXED

(Core :: XPCOM, task)

Description

[Simon Giesecke [:sg] [he/him]]

nsBaseHashtable::Get returns UserDataType by value, while nsBaseHashtable::GetOrInsert returns a reference to the actual DataType& (and thus allows modifying the entry). This is very confusing, given that both member functions are called Get. Either of these should be renamed to use a different term than Get .

Probably a method that just returns a reference to the existing value should be added as well to avoid uses as in Bug 1691910 or in TelemetryOrigin::RecordOrigin, which should never add a new entry.

Comment 1

[Simon Giesecke [:sg] [he/him]]

One option is to rename GetOrInsert to LookupOrInsert, which somehow fits in with existing Lookup* methods that also expose DataType&.

Comment 2

[Simon Giesecke [:sg] [he/him]]

nsClassHashtable::LookupOrAdd should be renamed along as well, maybe to LookupOrInsertNew (it does the same as LookupOrInsert, but uses the arguments to construct an object on the heap).

Comment 3

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1691913 - Fix forwarding of constructor arguments to nsBaseHashtableET. r=#xpcom-reviewers

Depends on D105333

Comment 4

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1691913 - Rename nsClassHashtable::LookupOrAdd to GetOrInsertNew. r=#xpcom-reviewers

It should be called "Get" rather than "Lookup" because it returns
UserDataType. "Add" is called "Insert" in the other methods.

Depends on D105469

Comment 5

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1691913 - Rename nsBaseHashtable::GetOrInsert(With) to LookupOrInsert(With). r=#xpcom-reviewers

The functions should be called "Lookup" rather than "Get" because they return
a DataType& (rather than UserDataType).

Depends on D105471

Comment 6

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1691913 - Rename nsBaseHashtable::Put to InsertOrUpdate. r=#xpcom-reviewers

This makes the naming more consistent with other functions called
Insert and/or Update. Also, it removes the ambiguity whether
Put expects that an entry already exists or not, in particular because
it differed from nsTHashtable::PutEntry in that regard.

Depends on D105472

Comment 7

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1691913 - Mark nsBaseHashtable (and its subclasses') member functions nodiscard where appropriate. r=#xpcom-reviewers

Depends on D105473

Comment 8

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1691913 - Add some convenience methods to LookupResult and EntryHandle. r=#xpcom-reviewers

Depends on D105474

Comment 9

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1691913 - Remove uses of nsDataHashtable::GetValue. r=#xpcom-reviewers

Depends on D105475

Comment 10

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1691913 - Remove custom nsDataHashtable::GetValue. r=#xpcom-reviewers

Depends on D105476

Comment 11

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1691913 - Update documentation of nsBaseHashtable. r=#xpcom-reviewers

Depends on D105477

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a8f6d80352c4
Fix forwarding of constructor arguments to nsBaseHashtableET. r=xpcom-reviewers,nika
https://hg.mozilla.org/integration/autoland/rev/c4e0b884258d
Rename nsClassHashtable::LookupOrAdd to GetOrInsertNew. r=xpcom-reviewers,nika

Comment 13

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/a8f6d80352c4
https://hg.mozilla.org/mozilla-central/rev/c4e0b884258d

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f4b677bf05f6
Rename nsBaseHashtable::GetOrInsert(With) to LookupOrInsert(With). r=xpcom-reviewers,necko-reviewers,jgilbert,dragana,nika
https://hg.mozilla.org/integration/autoland/rev/ff46eda9cc31
Rename nsBaseHashtable::Put to InsertOrUpdate. r=xpcom-reviewers,necko-reviewers,jgilbert,dragana,nika

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cf2c619858c1
Mark nsBaseHashtable (and its subclasses') member functions nodiscard where appropriate. r=xpcom-reviewers,necko-reviewers,dragana,nika

Comment 16

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/f4b677bf05f6
https://hg.mozilla.org/mozilla-central/rev/ff46eda9cc31
https://hg.mozilla.org/mozilla-central/rev/cf2c619858c1

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4ea48ba52014
Add some convenience methods to LookupResult and EntryHandle. r=xpcom-reviewers,nika
https://hg.mozilla.org/integration/autoland/rev/cff817b06e3e
Remove uses of nsDataHashtable::GetValue. r=xpcom-reviewers,necko-reviewers,dragana,nika
https://hg.mozilla.org/integration/autoland/rev/c503aa8bf0a1
Remove custom nsDataHashtable::GetValue. r=xpcom-reviewers,nika
https://hg.mozilla.org/integration/autoland/rev/50804b10b092
Update documentation of nsBaseHashtable. r=xpcom-reviewers,nika

Comment 18

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/4ea48ba52014
https://hg.mozilla.org/mozilla-central/rev/cff817b06e3e
https://hg.mozilla.org/mozilla-central/rev/c503aa8bf0a1
https://hg.mozilla.org/mozilla-central/rev/50804b10b092