All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jeff King <peff@peff.net>
To: git@vger.kernel.org
Cc: "brian m. carlson" <sandals@crustytoothpaste.net>,
	Derrick Stolee <stolee@gmail.com>,
	Jacob Keller <jacob.keller@gmail.com>
Subject: [PATCH 7/9] convert hashmap comparison functions to oideq()
Date: Sat, 25 Aug 2018 04:14:09 -0400	[thread overview]
Message-ID: <20180825081409.GG737@sigill.intra.peff.net> (raw)
In-Reply-To: <20180825080031.GA32139@sigill.intra.peff.net>

The comparison functions used for hashmaps don't care about
strict ordering; they only want to compare entries for
equality. Let's use the oideq() function instead, which can
potentially be better optimized. Note that unlike the
previous patches mass-converting calls like "!oidcmp()",
this patch could actually provide an improvement even with
the current implementation. Those comparison functions are
passed around as function pointers, so at compile-time the
compiler cannot realize that the caller (which is in another
file completely) will treat the return value as a boolean.

Note that this does change the return values in quite a
subtle way (it's still an int, but now the sign bit is
irrelevant for ordering). Because of their funny
hashmap-specific signature, it's unlikely that any of these
static functions would be reused for more generic ordering.
But to be double-sure, let's stop using "cmp" in their
names.

Calling them "eq" doesn't quite work either, because the
hashmap convention is actually _inverted_. "0" means "same",
and non-zero means "different". So I've called them "neq" by
convention here.

Signed-off-by: Jeff King <peff@peff.net>
---
I actually think the hashmap inversion is vaguely insane
and I'd be happy to see it flipped. But that's definitely
outside the scope of this patch. It would take some care to
do so in a way that the compiler would catch topics in
flight, since the signature would not change (and nor would
changing the name "cmpfn" in the hash struct help, because
it is usually filled in by passing to hashmap_init()).

 builtin/describe.c |  6 +++---
 oidmap.c           | 12 ++++++------
 patch-ids.c        |  8 ++++----
 3 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/builtin/describe.c b/builtin/describe.c
index 1e7c75ba82..22c0541da5 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -62,7 +62,7 @@ static const char *prio_names[] = {
 	N_("head"), N_("lightweight"), N_("annotated"),
 };
 
-static int commit_name_cmp(const void *unused_cmp_data,
+static int commit_name_neq(const void *unused_cmp_data,
 			   const void *entry,
 			   const void *entry_or_key,
 			   const void *peeled)
@@ -70,7 +70,7 @@ static int commit_name_cmp(const void *unused_cmp_data,
 	const struct commit_name *cn1 = entry;
 	const struct commit_name *cn2 = entry_or_key;
 
-	return oidcmp(&cn1->peeled, peeled ? peeled : &cn2->peeled);
+	return !oideq(&cn1->peeled, peeled ? peeled : &cn2->peeled);
 }
 
 static inline struct commit_name *find_commit_name(const struct object_id *peeled)
@@ -596,7 +596,7 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 		return cmd_name_rev(args.argc, args.argv, prefix);
 	}
 
-	hashmap_init(&names, commit_name_cmp, NULL, 0);
+	hashmap_init(&names, commit_name_neq, NULL, 0);
 	for_each_rawref(get_name, NULL);
 	if (!hashmap_get_size(&names) && !always)
 		die(_("No names found, cannot describe anything."));
diff --git a/oidmap.c b/oidmap.c
index d9fb19ba6a..b0841a0f58 100644
--- a/oidmap.c
+++ b/oidmap.c
@@ -1,14 +1,14 @@
 #include "cache.h"
 #include "oidmap.h"
 
-static int cmpfn(const void *hashmap_cmp_fn_data,
-		 const void *entry, const void *entry_or_key,
-		 const void *keydata)
+static int oidmap_neq(const void *hashmap_cmp_fn_data,
+		      const void *entry, const void *entry_or_key,
+		      const void *keydata)
 {
 	const struct oidmap_entry *entry_ = entry;
 	if (keydata)
-		return oidcmp(&entry_->oid, (const struct object_id *) keydata);
-	return oidcmp(&entry_->oid,
+		return !oideq(&entry_->oid, (const struct object_id *) keydata);
+	return !oideq(&entry_->oid,
 		      &((const struct oidmap_entry *) entry_or_key)->oid);
 }
 
@@ -21,7 +21,7 @@ static int hash(const struct object_id *oid)
 
 void oidmap_init(struct oidmap *map, size_t initial_size)
 {
-	hashmap_init(&map->map, cmpfn, NULL, initial_size);
+	hashmap_init(&map->map, oidmap_neq, NULL, initial_size);
 }
 
 void oidmap_free(struct oidmap *map, int free_entries)
diff --git a/patch-ids.c b/patch-ids.c
index 8f7c25d5db..960ea24054 100644
--- a/patch-ids.c
+++ b/patch-ids.c
@@ -28,14 +28,14 @@ int commit_patch_id(struct commit *commit, struct diff_options *options,
 /*
  * When we cannot load the full patch-id for both commits for whatever
  * reason, the function returns -1 (i.e. return error(...)). Despite
- * the "cmp" in the name of this function, the caller only cares about
+ * the "neq" in the name of this function, the caller only cares about
  * the return value being zero (a and b are equivalent) or non-zero (a
  * and b are different), and returning non-zero would keep both in the
  * result, even if they actually were equivalent, in order to err on
  * the side of safety.  The actual value being negative does not have
  * any significance; only that it is non-zero matters.
  */
-static int patch_id_cmp(const void *cmpfn_data,
+static int patch_id_neq(const void *cmpfn_data,
 			const void *entry,
 			const void *entry_or_key,
 			const void *unused_keydata)
@@ -53,7 +53,7 @@ static int patch_id_cmp(const void *cmpfn_data,
 	    commit_patch_id(b->commit, opt, &b->patch_id, 0))
 		return error("Could not get patch ID for %s",
 			oid_to_hex(&b->commit->object.oid));
-	return oidcmp(&a->patch_id, &b->patch_id);
+	return !oideq(&a->patch_id, &b->patch_id);
 }
 
 int init_patch_ids(struct patch_ids *ids)
@@ -63,7 +63,7 @@ int init_patch_ids(struct patch_ids *ids)
 	ids->diffopts.detect_rename = 0;
 	ids->diffopts.flags.recursive = 1;
 	diff_setup_done(&ids->diffopts);
-	hashmap_init(&ids->patches, patch_id_cmp, &ids->diffopts, 256);
+	hashmap_init(&ids->patches, patch_id_neq, &ids->diffopts, 256);
 	return 0;
 }
 
-- 
2.19.0.rc0.412.g7005db4e88


  parent reply	other threads:[~2018-08-25  8:14 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-08-25  8:00 [PATCH 0/9] introducing oideq() Jeff King
2018-08-25  8:03 ` [PATCH 1/9] coccinelle: use <...> for function exclusion Jeff King
2018-08-25  8:05 ` [PATCH 2/9] introduce hasheq() and oideq() Jeff King
2018-08-25 10:58   ` Andrei Rybak
2018-08-25  8:07 ` [PATCH 3/9] convert "oidcmp() == 0" to oideq() Jeff King
2018-08-25  8:36   ` Jeff King
2018-08-27 12:31     ` Derrick Stolee
2018-08-27 12:33       ` Derrick Stolee
2018-08-25  8:08 ` [PATCH 4/9] convert "hashcmp() == 0" to hasheq() Jeff King
2018-08-25  8:09 ` [PATCH 5/9] convert "oidcmp() != 0" to "!oideq()" Jeff King
2018-08-25  8:10 ` [PATCH 6/9] convert "hashcmp() != 0" to "!hasheq()" Jeff King
2018-08-25  8:14 ` Jeff King [this message]
2018-08-25  8:15 ` [PATCH 8/9] read-cache: use oideq() in ce_compare functions Jeff King
2018-08-25  8:17 ` [PATCH 9/9] show_dirstat: simplify same-content check Jeff King
2018-08-25  8:20   ` Eric Sunshine
2018-08-25  8:23     ` Jeff King
2018-08-26 20:56 ` [PATCH 0/9] introducing oideq() brian m. carlson
2018-08-27 12:41   ` Derrick Stolee
2018-08-28 21:21   ` Jeff King
2018-08-28 21:22     ` [PATCH v2 1/9] coccinelle: use <...> for function exclusion Jeff King
2018-08-28 21:22     ` [PATCH v2 2/9] introduce hasheq() and oideq() Jeff King
2018-08-28 21:22     ` [PATCH v2 3/9] convert "oidcmp() == 0" to oideq() Jeff King
2018-08-28 21:22     ` [PATCH v2 4/9] convert "hashcmp() == 0" to hasheq() Jeff King
2018-08-28 21:22     ` [PATCH v2 5/9] convert "oidcmp() != 0" to "!oideq()" Jeff King
2018-08-28 21:22     ` [PATCH v2 6/9] convert "hashcmp() != 0" to "!hasheq()" Jeff King
2018-08-28 21:22     ` [PATCH v2 7/9] convert hashmap comparison functions to oideq() Jeff King
2018-08-28 21:22     ` [PATCH v2 8/9] read-cache: use oideq() in ce_compare functions Jeff King
2018-08-28 21:23     ` [PATCH v2 9/9] show_dirstat: simplify same-content check Jeff King
2018-08-28 21:36     ` [PATCH 0/9] introducing oideq() Derrick Stolee
2018-08-29  0:08     ` brian m. carlson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180825081409.GG737@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=jacob.keller@gmail.com \
    --cc=sandals@crustytoothpaste.net \
    --cc=stolee@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.