Closed Bug 708901 Opened 10 years ago Closed 3 months ago

have an nsTHashtable derivative that's designed for storing sets

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
89 Branch
Tracking Status
firefox89 --- fixed

People

(Reporter: dbaron, Assigned: sg)

References

Details

Attachments

(38 files, 1 obsolete file)

17.01 KB, patch
Details | Diff | Splinter Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
Attached patch patchSplinter Review
Some people use nsTHashtable to store sets, i.e., hashtables that have keys but no values.  However, it's a rather awkward API to use directly, as the patches in bug 700659 show -- the method names don't really correspond to the method names you'd expect for a set.

I wrote a class derived from nsTHashtable that provides a better set API quite a while ago -- I'd used it as part of 2 patches I was working on, but both times it wasn't needed by the end of the patch.

It's a fork of nsBaseHashtable -- so it provides an API more similar to the way nsTHashtable gets used when it's used for key-value pairs.  The differences are really quite small.   Essentially instead of:
  bool Get(KeyType, UserDataType*);
  UserDataType Get(KeyType); // single-threaded version only (why?)
  bool Put(KeyType, UserDataType);
it has:
  bool Has(KeyType)
  bool Add(KeyType);
and then the enumeration function has one fewer argument since there's no value.

This can let us avoid using PutEntry, RemoveEntry, and !!GetEntry as a set API:  instead we have Add, Remove, and Has.
Attachment #580241 - Flags: review?(benjamin)
Assignee: nobody → dbaron
Can we just add these methods to nsTHashtable, and if appropriate typedef it? See also bug 700569.
i.e. typedef nsTHashtable<EntryType> nsHashSet<EntryType> to make it clear what consumers intend.
That should be bug 700659
Comment on attachment 580241 [details] [diff] [review]
patch

Clearing flag for now, I think we can do the simpler way I mentioned. re-request review if that's not good for some reason.
Attachment #580241 - Flags: review?(benjamin)
Assignee: dbaron → nobody

I think :sg is looking into doing something like this now, so adding a ni?

Flags: needinfo?(sgiesecke)

Depends on D107683

Assignee: nobody → sgiesecke
Status: NEW → ASSIGNED
See Also: → 1697377

Depends on D108588

Depends on D108589

Depends on D108590

Depends on D108592

Depends on D108596

Depends on D108597

Depends on D108599

Depends on D108600

Depends on D108601

Depends on D108602

Attachment #9209398 - Attachment description: Bug 708901 - WIP: Migrate to nsTHashSet in accessible. → Bug 708901 - Migrate to nsTHashSet in accessible.
Attachment #9209399 - Attachment description: Bug 708901 - WIP: Migrate to nsTHashSet in caps. → Bug 708901 - Migrate to nsTHashSet in caps.
Attachment #9209400 - Attachment description: Bug 708901 - WIP: Migrate to nsTHashSet in docshell. → Bug 708901 - Migrate to nsTHashSet in docshell.
Attachment #9209402 - Attachment description: Bug 708901 - WIP: Migrate to nsTHashSet in dom/animation. → Bug 708901 - Migrate to nsTHashSet in dom/animation.
Attachment #9209403 - Attachment description: Bug 708901 - WIP: Migrate to nsTHashSet in dom/base. → Bug 708901 - Migrate to nsTHashSet in dom/base.
Attachment #9209404 - Attachment description: Bug 708901 - WIP: Migrate to nsTHashSet in dom/filehandle. → Bug 708901 - Migrate to nsTHashSet in dom/filehandle.
Attachment #9209406 - Attachment description: Bug 708901 - WIP: Migrate to nsTHashSet in dom/filesystem. → Bug 708901 - Migrate to nsTHashSet in dom/filesystem.
Attachment #9209407 - Attachment description: Bug 708901 - WIP: Migrate to nsTHashSet in dom/indexedDB. r=#dom-storage → Bug 708901 - Migrate to nsTHashSet in dom/indexedDB. r=#dom-storage
Attachment #9209409 - Attachment description: Bug 708901 - WIP: Migrate to nsTHashSet in layout. → Bug 708901 - Migrate to nsTHashSet in layout.
Attachment #9209410 - Attachment description: Bug 708901 - WIP: Migrate to nsTHashSet in dom/ipc. → Bug 708901 - Migrate to nsTHashSet in dom/ipc.
Attachment #9209411 - Attachment description: Bug 708901 - WIP: Migrate to nsTHashSet in dom/localstorage. → Bug 708901 - Migrate to nsTHashSet in dom/localstorage.
Attachment #9209412 - Attachment description: Bug 708901 - WIP: Migrate to nsTHashSet in dom/quota. → Bug 708901 - Migrate to nsTHashSet in dom/quota.
Attachment #9209413 - Attachment description: Bug 708901 - WIP: Migrate to nsTHashSet in netwerk. → Bug 708901 - Migrate to nsTHashSet in netwerk.
Attachment #9209414 - Attachment description: Bug 708901 - WIP: Migrate to nsTHashSet in uriloader. → Bug 708901 - Migrate to nsTHashSet in uriloader.
Attachment #9209415 - Attachment description: Bug 708901 - WIP: Migrate to nsTHashSet in gfx/thebes. → Bug 708901 - Migrate to nsTHashSet in gfx/thebes.

Depends on D108603

Depends on D109316

Depends on D109317

Depends on D109318

Depends on D109319

Depends on D109320

Depends on D109322

Depends on D109325

Depends on D108588

Depends on D109329

Depends on D109330

Depends on D109331

Attachment #9209416 - Attachment description: Bug 708901 - WIP: Migrate to nsTHashSet. → Bug 708901 - Migrate to nsTHashSet in toolkit/components/url-classifier.
Attachment #9210751 - Attachment is obsolete: true
Flags: needinfo?(sgiesecke)
Attachment #9210734 - Attachment description: Bug 708901 - Migrate to nsTHashSet in toolkit/components/extension. r=#extension-reviewers → Bug 708901 - Migrate to nsTHashSet in toolkit/components/extensions. r=#extension-reviewers
Pushed by sgiesecke@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f0b2778adb4e
Add nsTHashSet. r=xpcom-reviewers,nika
https://hg.mozilla.org/integration/autoland/rev/1ce2d2fcd135
Migrate to nsTHashSet in toolkit/components/telemetry/core. r=chutten
https://hg.mozilla.org/integration/autoland/rev/b9b830793722
Migrate to nsTHashSet in accessible. r=Jamie
https://hg.mozilla.org/integration/autoland/rev/c71df025183f
Migrate to nsTHashSet in dom/animation. r=birtles
https://hg.mozilla.org/integration/autoland/rev/98699e865285
Migrate to nsTHashSet in dom/filehandle. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/ded9b5290465
Migrate to nsTHashSet in dom/filesystem. r=baku
https://hg.mozilla.org/integration/autoland/rev/975f08e5f470
Migrate to nsTHashSet in dom/indexedDB. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/7894c271cbd4
Migrate to nsTHashSet in dom/ipc. r=nika
https://hg.mozilla.org/integration/autoland/rev/3b7de745637f
Migrate to nsTHashSet in dom/localstorage. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/522fe081804b
Migrate to nsTHashSet in dom/quota. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/00d1e5656958
Migrate to nsTHashSet in netwerk. r=necko-reviewers,valentin
https://hg.mozilla.org/integration/autoland/rev/570a3dbde1a5
Migrate to nsTHashSet in gfx/thebes. r=jfkthame,jrmuizel
https://hg.mozilla.org/integration/autoland/rev/fd64d5ee8565
Migrate to nsTHashSet in image. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/7eebc4ce84ea
Migrate to nsTHashSet in gfx/ipc. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/e97b1357a056
Migrate to nsTHashSet in gfx/layers. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/207fce4edd19
Migrate to nsTHashSet in dom/l10n. r=zbraniecki
https://hg.mozilla.org/integration/autoland/rev/7c81249f9d9a
Migrate to nsTHashSet in dom/media. r=padenot
https://hg.mozilla.org/integration/autoland/rev/1e48b071cd6d
Migrate to nsTHashSet in dom/serviceworkers. r=dom-worker-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/e8c72f1f94f7
Migrate to nsTHashSet in startupcache. r=dthayer
https://hg.mozilla.org/integration/autoland/rev/674e0fa33608
Migrate to nsTHashSet in xpcom. r=xpcom-reviewers,kmag
https://hg.mozilla.org/integration/autoland/rev/e578b5720943
Migrate to nsTHashSet in toolkit/components/extensions. r=extension-reviewers,zombie
https://hg.mozilla.org/integration/autoland/rev/8c3a523ee4ea
Migrate to nsTHashSet in toolkit/components/url-classifier. r=gcp
https://hg.mozilla.org/integration/autoland/rev/06b5568225ed
Migrate to nsTHashSet in dom/xslt. r=smaug
Attachment #9209403 - Attachment description: Bug 708901 - Migrate to nsTHashSet in dom/base. → WIP: Bug 708901 - Migrate to nsTHashSet in dom/base.
Attachment #9209403 - Attachment description: WIP: Bug 708901 - Migrate to nsTHashSet in dom/base. → Bug 708901 - Migrate to nsTHashSet in dom/base.
Pushed by sgiesecke@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e25691b542b5
Migrate to nsTHashSet in parser. r=smaug
https://hg.mozilla.org/integration/autoland/rev/77c02e07a11a
Migrate to nsTHashSet in dom/plugins. r=smaug
Pushed by sgiesecke@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/74d8249fbe7e
Migrate to nsTHashSet in caps. r=ckerschb
https://hg.mozilla.org/integration/autoland/rev/aed22fd80893
Migrate to nsTHashSet in docshell. r=smaug
https://hg.mozilla.org/integration/autoland/rev/73f11d3c1298
Migrate to nsTHashSet in dom/base. r=emilio
https://hg.mozilla.org/integration/autoland/rev/03c3a56c3d13
Migrate to nsTHashSet in layout. r=emilio
https://hg.mozilla.org/integration/autoland/rev/fc54f4eaedd5
Migrate to nsTHashSet in uriloader. r=valentin
https://hg.mozilla.org/integration/autoland/rev/be31ccd9a61d
Migrate to nsTHashSet in gfx/vr. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/15a8fb94d15d
Migrate to nsTHashSet in ipc. r=ipc-reviewers,nika
https://hg.mozilla.org/integration/autoland/rev/479bf64c7986
Migrate to nsTHashSet in dom/storage. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/eee75f33f060
Migrate to nsTHashSet in dom/grid. r=mats
https://hg.mozilla.org/integration/autoland/rev/37b52cce83c0
Migrate to nsTHashSet in extensions/permissions. r=timhuang
https://hg.mozilla.org/integration/autoland/rev/b1e4c01e63b8
Migrate to nsTHashSet in extensions/spellcheck. r=masayuki

Backed out along with Bug 1184468 for causing build bustage on GeckoViewHistory.cpp

backout: https://hg.mozilla.org/integration/autoland/rev/a29c896b00338ce493b1890c199019416545545f

push: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&selectedTaskRun=TiHlUywHQiuKoaLhZHFOEA.0&revision=b1e4c01e63b8ed2f28b907dd862f951a136f54de&searchStr=build

failure log: https://treeherder.mozilla.org/logviewer?job_id=334304057&repo=autoland&lineNumber=18748

[task 2021-03-24T17:21:25.360Z] 17:21:25     INFO -  /builds/worker/fetches/sccache/sccache /builds/worker/fetches/clang/bin/clang++ -std=gnu++17 --target=aarch64-linux-android -o GeckoViewHistory.o -c  -I/builds/worker/workspace/obj-build/dist/stl_wrappers -I/builds/worker/workspace/obj-build/dist/system_wrappers -include /builds/worker/checkouts/gecko/config/gcc_hidden.h -fstack-protector-strong -ftrivial-auto-var-init=pattern -DDEBUG=1 -DOS_POSIX=1 -DOS_LINUX=1 -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -DSTATIC_EXPORTABLE_JS_API -I/builds/worker/checkouts/gecko/mobile/android/components/geckoview -I/builds/worker/workspace/obj-build/mobile/android/components/geckoview -I/builds/worker/workspace/obj-build/ipc/ipdl/_ipdlheaders -I/builds/worker/checkouts/gecko/ipc/chromium/src -I/builds/worker/checkouts/gecko/ipc/glue -I/builds/worker/workspace/obj-build/dist/include -I/builds/worker/workspace/obj-build/dist/include/nspr -I/builds/worker/workspace/obj-build/dist/include/nss -DMOZILLA_CLIENT -include /builds/worker/workspace/obj-build/mozilla-config.h -Qunused-arguments -isystem /builds/worker/fetches/android-ndk/sysroot/usr/include/aarch64-linux-android -isystem /builds/worker/fetches/android-ndk/sysroot/usr/include -gcc-toolchain /builds/worker/fetches/android-ndk/toolchains/aarch64-linux-android-4.9/prebuilt/linux-x86_64 -D__ANDROID_API__=21 -Qunused-arguments -Wall -Wbitfield-enum-conversion -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wshadow-field-in-constructor-modified -Wsign-compare -Wtype-limits -Wunreachable-code -Wunreachable-code-return -Wwrite-strings -Wno-invalid-offsetof -Wclass-varargs -Wempty-init-stmt -Wfloat-overflow-conversion -Wfloat-zero-conversion -Wloop-analysis -Wno-range-loop-analysis -Wc++2a-compat -Wcomma -Wimplicit-fallthrough -Wunused-function -Wunused-variable -Werror=non-literal-null-conversion -Wstring-conversion -Wtautological-overlap-compare -Wtautological-unsigned-enum-zero-compare -Wtautological-unsigned-zero-compare -Wno-error=tautological-type-limit-compare -Wno-inline-new-delete -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=backend-plugin -Wno-error=return-std-move -Wno-error=atomic-alignment -Wno-error=deprecated-copy -Wformat -Wformat-security -Wno-gnu-zero-variadic-macro-arguments -Wno-psabi -Wno-unknown-warning-option -fno-sized-deallocation -fno-aligned-new -fno-short-enums -fno-exceptions -fcrash-diagnostics-dir=/builds/worker/artifacts -stdlib=libstdc++ -I/builds/worker/fetches/android-ndk/sources/cxx-stl/llvm-libc++/include -I/builds/worker/fetches/android-ndk/sources/android/support/include -I/builds/worker/fetches/android-ndk/sources/cxx-stl/llvm-libc++abi/include -fno-exceptions -fno-strict-aliasing -fPIC -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pipe -g -Xclang -load -Xclang /builds/worker/workspace/obj-build/build/clang-plugin/libclang-plugin.so -Xclang -add-plugin -Xclang moz-check -Oz -mno-outline -fno-omit-frame-pointer -funwind-tables -Werror -fexperimental-new-pass-manager  -MD -MP -MF .deps/GeckoViewHistory.o.pp   /builds/worker/checkouts/gecko/mobile/android/components/geckoview/GeckoViewHistory.cpp
[task 2021-03-24T17:21:25.360Z] 17:21:25    ERROR -  /builds/worker/checkouts/gecko/mobile/android/components/geckoview/GeckoViewHistory.cpp:87:30: error: 'ConstIter' is a protected member of 'nsTHashtable<nsURIHashKey>'
[task 2021-03-24T17:21:25.360Z] 17:21:25     INFO -    for (auto query = aQueries.ConstIter(); !query.Done(); query.Next()) {
[task 2021-03-24T17:21:25.360Z] 17:21:25     INFO -                               ^
[task 2021-03-24T17:21:25.360Z] 17:21:25     INFO -  /builds/worker/workspace/obj-build/dist/include/nsTHashSet.h:21:24: note: constrained by protected inheritance here
[task 2021-03-24T17:21:25.360Z] 17:21:25     INFO -  class nsTBaseHashSet : protected nsTHashtable<KeyClass> {
[task 2021-03-24T17:21:25.360Z] 17:21:25     INFO -                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[task 2021-03-24T17:21:25.360Z] 17:21:25     INFO -  /builds/worker/workspace/obj-build/dist/include/nsTHashtable.h:505:17: note: member is declared here
[task 2021-03-24T17:21:25.360Z] 17:21:25     INFO -    ConstIterator ConstIter() const {
[task 2021-03-24T17:21:25.360Z] 17:21:25     INFO -                  ^
Flags: needinfo?(sgiesecke)
Pushed by sgiesecke@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/634e93679bfa
Migrate to nsTHashSet in caps. r=ckerschb
https://hg.mozilla.org/integration/autoland/rev/185677e00b4f
Migrate to nsTHashSet in dom/base. r=emilio
https://hg.mozilla.org/integration/autoland/rev/e1624e209c43
Migrate to nsTHashSet in layout. r=emilio
https://hg.mozilla.org/integration/autoland/rev/72accaecbbb6
Migrate to nsTHashSet in uriloader. r=valentin
https://hg.mozilla.org/integration/autoland/rev/2d143bf359ee
Migrate to nsTHashSet in gfx/vr. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/b2e334974f9b
Migrate to nsTHashSet in ipc. r=ipc-reviewers,nika
https://hg.mozilla.org/integration/autoland/rev/e47dbfe1a770
Migrate to nsTHashSet in dom/storage. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/ebe6536a936f
Migrate to nsTHashSet in dom/grid. r=mats
https://hg.mozilla.org/integration/autoland/rev/79950663fcd8
Migrate to nsTHashSet in extensions/permissions. r=timhuang
https://hg.mozilla.org/integration/autoland/rev/ffcd3238d413
Migrate to nsTHashSet in extensions/spellcheck. r=masayuki
Pushed by sgiesecke@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d3ff45a05f6a
Migrate to nsTHashSet in docshell. r=smaug,geckoview-reviewers,aklotz
Flags: needinfo?(simon.giesecke)
You need to log in before you can comment on or make changes to this bug.