Closed Bug 708901 Opened 11 years ago Closed 1 year 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

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 D109318

Depends on D109319

Depends on D109320

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.