Page MenuHome
Differential D1179

GHash - Add 'set' operations & checks
AbandonedPublic
Actions

Authored by Bastien Montagne (mont29) on Mar 13 2015, 1:19 PM.

Details

Reviewers
None
Maniphest Tasks
T43766: GHash Improvements
Commits
rB94549adec4b6: Merge branch 'master' into temp-ghash-setops
rB77a4d705c7fe: Merge branch 'master' into temp-ghash-setops
rBf808f36c1231: Get rid of varargs in union & co functions (using variadic macros instead).
rBb6f0c9e25ad8: Some improvements/tweaks from review.
rBecc9fe994592: Merge branch 'master' into temp-ghash-setops
Summary

Based on D1178

Not much to say here, this is pretty straight forward.

Note that we are “cool” with operations which only affect the first item (intersections and differences), and allow in those cases all other args to be gsets, even if first one is ghash. Reason here is that you may want to 'trim' a ghash based on some gset’s content…

Diff Detail

Repository
rB Blender

Event Timeline

Bastien Montagne (mont29) updated this revision to Diff 3737.Mar 13 2015, 1:19 PM
Bastien Montagne (mont29) retitled this revision from to GHash - Add 'set' operations & checks.
Bastien Montagne (mont29) updated this object.
Bastien Montagne (mont29) added a reviewer: Campbell Barton (campbellbarton).
Bastien Montagne (mont29) set the repository for this revision to rB Blender.
Bastien Montagne (mont29) added a project: BF Blender.
Bastien Montagne (mont29) added subscribers: Sergey Sharybin (sergey), Lukas Tönne (lukastoenne), Antonis Ryakiotakis (psy-fi).
Bastien Montagne (mont29) mentioned this in D1180: GHash - Add GTests.Mar 13 2015, 1:21 PM
Bastien Montagne (mont29) mentioned this in D1181: GHash - add optional hash storage in entries.Mar 13 2015, 1:25 PM
Bastien Montagne (mont29) added a task: T43766: GHash Improvements.Mar 13 2015, 2:10 PM
Campbell Barton (campbellbarton) requested changes to this revision.Mar 18 2015, 3:42 PM
Campbell Barton (campbellbarton) edited edge metadata.

Having set operations (union/subset) GHash I dont think makes so much sense and is even misleading. union for example needs to select values from one of the GHash's. So would prefer just to call this merge (then option if you want values from left/right).

This is request to remove quite some code but would really to start off this patch by adding utility gset operations on sets and add extra utility functions later.

This revision now requires changes to proceed.Mar 18 2015, 3:42 PM
Bastien Montagne (mont29) added a comment.Mar 18 2015, 3:55 PM

I really disagree here… :)

GHash is gset with data, so I do not see why we would not want to be able to perform the same operations as on gset. Especially since it adds virtually no more code (just the few bits needed to copy or free values in addition to keys), the 'hard' part (i.e. the operation itself) is exactly the same for both types.

Actually, even with gset, Union is also a merge, since we have to decide whether we keep keys from left or right side. This is pure semantic stuff, still prefer to keep same naming on both gset and ghash, it enforces the fact that they use same code in the end, and also makes using those operations simpler/clearer imho.

Bastien Montagne (mont29) mentioned this in D1155: GHash enhancements..Mar 19 2015, 9:12 PM
Bastien Montagne (mont29) updated this revision to Diff 3784.Mar 20 2015, 12:10 PM
Bastien Montagne (mont29) edited edge metadata.
Bastien Montagne (mont29) updated this object.

Updated from master, and removed public GHash API for union/inter/diff/symmdiff.

Dead easy to add them back (with those 'setops' names or others like merge & co)
later, anyway. Core code remains unchanged.

Note I kept pure key checks (isdisjoint, etc.) for ghashes, those are not an issue
at all (I hope :p ), it's equivalent to performing setops on dict.keys() views in py...

Campbell Barton (campbellbarton) added a comment.EditedMar 20 2015, 12:32 PM

eeeh, well seems we have to agree to really disagree :S

In general the checking functions make some sense to have bothe ghash/gset functions - since they dont create new data, BLI_ghash_is_subset_keys, BLI_ghash_is_equal_keys - for eg.

But functions which create new data - these should only have set versions - (subset, union, intersection - etc).

If one day we need a ghash version of a set function, we can check on having one then.

source/blender/blenlib/intern/BLI_ghash.c
606–610

Would rather just call this a ghash_merge or ghash_update

1230

There could be a fast-path here, if both have the same number of buckets (which isn't so unlikely seeing as they are the same size).

Also goes for other functions here.

tests/gtests/blenlib/BLI_ghash_test.cc
258 ↗(On Diff #3784)

should call gset, Union?, same for Difference

Campbell Barton (campbellbarton) added inline comments.Mar 20 2015, 12:39 PM
source/blender/blenlib/intern/BLI_ghash.c
1743

minor note regarding var-args.
We could avoid using va args and instead use a macro...

#define BLI_gset_union(keycopyfp, ...) \
{ \
    GSet *gset_arr[] = {__VA_ARGS__}; \
    _bli_gset_union(keycopyfp, gset_arr, ARRAY_SIZE(gset_arr)); \
} (void)0

This way its type-safe ans no need for a NULL terminator arg.

Bastien Montagne (mont29) updated this revision to Diff 3788.Mar 20 2015, 4:58 PM
Bastien Montagne (mont29) edited edge metadata.
  • Some improvements/tweaks from review.
  • Get rid of varargs in union & co functions (using variadic macros instead).
Closed by commit rBecc9fe994592: Merge branch 'master' into temp-ghash-setops (authored by Bastien Montagne (mont29)). · Explain WhyMar 20 2015, 4:58 PM
This revision was automatically updated to reflect the committed changes.
Bastien Montagne (mont29) reopened this revision.Mar 20 2015, 5:00 PM

<known sentence>

Campbell Barton (campbellbarton) added a comment.Mar 21 2015, 6:43 AM

some mixup happened on update, this is a sequencer patch

Bastien Montagne (mont29) updated this revision to Diff 3791.Mar 21 2015, 10:13 AM
  • Some improvements/tweaks from review.
  • Get rid of varargs in union & co functions (using variadic macros instead).
Bastien Montagne (mont29) added a comment.Mar 21 2015, 10:14 AM

Stupid phab!

Sergey Sharybin (sergey) added a comment.Mar 25 2015, 4:11 PM

That indeed seems a bit weird to have set operations on hash, it's just asking for problems and confusion in the future. For example, is {key: 1, value: foo} equals to {key 1:, value: bar}? Such things are really an easy way to shoot self into own foot.

Would just leave those functions aside for until we really need them and wouldn't add functions just because they seems easy to add, hoping they'll become handy and clear in the future.

Closed by commit rB94549adec4b6: Merge branch 'master' into temp-ghash-setops (authored by Bastien Montagne (mont29)). · Explain WhyJun 29 2015, 5:17 PM
This revision was automatically updated to reflect the committed changes.
Bastien Montagne (mont29) reopened this revision.Jun 29 2015, 5:23 PM

Let’s curse the bots…

Campbell Barton (campbellbarton) removed a reviewer: Campbell Barton (campbellbarton).Aug 11 2016, 6:06 AM
Bastien Montagne (mont29) abandoned this revision.Mar 14 2019, 10:09 AM

Revision Contents
Changeset List

PathSize
1 line
2 lines
3 lines
intern/
cycles/
bvh/
4 lines
ghost/
intern/
6 lines
source/
blender/
blenkernel/
4 lines
intern/
8 lines
2 lines
blenlib/
intern/
66 lines
2 lines
editors/
include/
8 lines
space_view3d/
39 lines
134 lines
gpu/
3 lines
render/
intern/
source/
4 lines

Diff 4537

View Options

CMakeLists.txt

Loading...
View Options

GNUmakefile

Loading...
View Options

SConstruct

Loading...
View Options

intern/cycles/bvh/bvh_params.h

Loading...
View Options

intern/ghost/intern/GHOST_ContextCGL.mm

Loading...
View Options

source/blender/blenkernel/BKE_camera.h

Loading...
View Options

source/blender/blenkernel/intern/camera.c

Loading...
View Options

source/blender/blenkernel/intern/tracking.c

Loading...
View Options

source/blender/blenlib/intern/BLI_ghash.c

Loading...
View Options

source/blender/blenlib/intern/hash_mm2a.c

Loading...
View Options

source/blender/editors/include/ED_view3d.h

Loading...
View Options

source/blender/editors/space_view3d/view3d_edit.c

Loading...
View Options

source/blender/editors/space_view3d/view3d_view.c

Loading...
View Options

source/blender/gpu/SConscript

Loading...
View Options

source/blender/render/intern/source/external_engine.c

Loading...