Re: [PATCH 3/9] convert "oidcmp() == 0" to oideq()
On 8/27/2018 8:31 AM, Derrick Stolee wrote: On 8/25/2018 4:36 AM, Jeff King wrote: On Sat, Aug 25, 2018 at 04:07:15AM -0400, Jeff King wrote: diff --git a/contrib/coccinelle/object_id.cocci b/contrib/coccinelle/object_id.cocci index 5869979be7..548c02336d 100644 --- a/contrib/coccinelle/object_id.cocci +++ b/contrib/coccinelle/object_id.cocci @@ -108,3 +108,9 @@ expression E1, E2; @@ - hashcpy(E1.hash, E2->hash) + oidcpy(, E2) Is this change intended? It doesn't seem to match the intention of the rest of the patch. Ignore me. It's just confusing to read the +/- notation from a cocci script alongside the file diff.
Re: [PATCH 3/9] convert "oidcmp() == 0" to oideq()
On 8/25/2018 4:36 AM, Jeff King wrote:
On Sat, Aug 25, 2018 at 04:07:15AM -0400, Jeff King wrote:
diff --git a/contrib/coccinelle/object_id.cocci
b/contrib/coccinelle/object_id.cocci
index 5869979be7..548c02336d 100644
--- a/contrib/coccinelle/object_id.cocci
+++ b/contrib/coccinelle/object_id.cocci
@@ -108,3 +108,9 @@ expression E1, E2;
@@
- hashcpy(E1.hash, E2->hash)
+ oidcpy(, E2)
Is this change intended? It doesn't seem to match the intention of the
rest of the patch.
+
+@@
+expression E1, E2;
+@@
+- oidcmp(E1, E2) == 0
++ oideq(E1, E2)
[...]
diff --git a/cache.h b/cache.h
index f6d227fac7..d090f71706 100644
--- a/cache.h
+++ b/cache.h
@@ -1100,7 +1100,7 @@ static inline int is_empty_blob_sha1(const unsigned char
*sha1)
static inline int is_empty_blob_oid(const struct object_id *oid)
{
- return !oidcmp(oid, the_hash_algo->empty_blob);
+ return oideq(oid, the_hash_algo->empty_blob);
}
By the way, one interesting thing I noticed is that coccinelle generates
the hunk for cache.h for _every_ file that includes it, and the
resulting patch is annoying to apply. I think this is related to the
discussion we were having in:
https://public-inbox.org/git/20180802115522.16107-1-szeder@gmail.com/
Namely that we might do better to invoke one big spatch (and let it
parallelize internally) instead of one perfile. Presumably that would
also avoid looking at the headers repeatedly (which would be both faster
and produce better output).
I'm not planning to pursue that immediately, so just food for thought at
this point.
The more we use Coccinelle, the more we will learn how to improve the
workflow.
Thanks,
-Stolee
Re: [PATCH 3/9] convert "oidcmp() == 0" to oideq()
On Sat, Aug 25, 2018 at 04:07:15AM -0400, Jeff King wrote:
> diff --git a/contrib/coccinelle/object_id.cocci
> b/contrib/coccinelle/object_id.cocci
> index 5869979be7..548c02336d 100644
> --- a/contrib/coccinelle/object_id.cocci
> +++ b/contrib/coccinelle/object_id.cocci
> @@ -108,3 +108,9 @@ expression E1, E2;
> @@
> - hashcpy(E1.hash, E2->hash)
> + oidcpy(, E2)
> +
> +@@
> +expression E1, E2;
> +@@
> +- oidcmp(E1, E2) == 0
> ++ oideq(E1, E2)
>
> [...]
>
> diff --git a/cache.h b/cache.h
> index f6d227fac7..d090f71706 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1100,7 +1100,7 @@ static inline int is_empty_blob_sha1(const unsigned
> char *sha1)
>
> static inline int is_empty_blob_oid(const struct object_id *oid)
> {
> - return !oidcmp(oid, the_hash_algo->empty_blob);
> + return oideq(oid, the_hash_algo->empty_blob);
> }
By the way, one interesting thing I noticed is that coccinelle generates
the hunk for cache.h for _every_ file that includes it, and the
resulting patch is annoying to apply. I think this is related to the
discussion we were having in:
https://public-inbox.org/git/20180802115522.16107-1-szeder@gmail.com/
Namely that we might do better to invoke one big spatch (and let it
parallelize internally) instead of one perfile. Presumably that would
also avoid looking at the headers repeatedly (which would be both faster
and produce better output).
I'm not planning to pursue that immediately, so just food for thought at
this point.
-Peff
[PATCH 3/9] convert "oidcmp() == 0" to oideq()
Using the more restrictive oideq() should, in the long run,
give the compiler more opportunities to optimize these
callsites. For now, this conversion should be a complete
noop with respect to the generated code.
The result is also perhaps a little more readable, as it
avoids the "zero is equal" idiom. Since it's so prevalent in
C, I think seasoned programmers tend not to even notice it
anymore, but it can sometimes make for awkward double
negations (e.g., we can drop a few !!oidcmp() instances
here).
This patch was generated almost entirely by the included
coccinelle patch. This mechanical conversion should be
completely safe, because we check explicitly for cases where
oidcmp() is compared to 0, which is what oideq() is doing
under the hood. Note that we don't have to catch "!oidcmp()"
separately; coccinelle's standard isomorphisms make sure the
two are treated equivalently.
I say "almost" because I did hand-edit the coccinelle output
to fix up a few style violations (it mostly keeps the
original formatting, but sometimes unwraps long lines).
Signed-off-by: Jeff King
---
bisect.c | 4 ++--
blame.c| 4 ++--
builtin/am.c | 2 +-
builtin/checkout.c | 2 +-
builtin/describe.c | 4 ++--
builtin/diff.c | 2 +-
builtin/difftool.c | 4 ++--
builtin/fast-export.c | 2 +-
builtin/fetch.c| 4 ++--
builtin/fmt-merge-msg.c| 2 +-
builtin/index-pack.c | 4 ++--
builtin/log.c | 6 +++---
builtin/merge-tree.c | 2 +-
builtin/merge.c| 4 ++--
builtin/pack-objects.c | 4 ++--
builtin/pull.c | 2 +-
builtin/receive-pack.c | 4 ++--
builtin/remote.c | 2 +-
builtin/replace.c | 6 +++---
builtin/unpack-objects.c | 2 +-
builtin/update-index.c | 4 ++--
bulk-checkin.c | 2 +-
cache-tree.c | 2 +-
cache.h| 4 ++--
combine-diff.c | 4 ++--
commit-graph.c | 2 +-
connect.c | 2 +-
contrib/coccinelle/object_id.cocci | 6 ++
diff-lib.c | 2 +-
diff.c | 6 +++---
diffcore-break.c | 2 +-
fast-import.c | 6 +++---
http-push.c| 2 +-
log-tree.c | 6 +++---
merge-recursive.c | 4 ++--
notes-merge.c | 24 +++---
notes.c| 4 ++--
pack-write.c | 2 +-
read-cache.c | 2 +-
ref-filter.c | 2 +-
refs/files-backend.c | 4 ++--
remote.c | 6 +++---
revision.c | 2 +-
sequencer.c| 32 +++---
sha1-array.c | 2 +-
sha1-file.c| 4 ++--
sha1-name.c| 2 +-
submodule.c| 2 +-
transport.c| 2 +-
unpack-trees.c | 6 +++---
wt-status.c| 10 +-
xdiff-interface.c | 2 +-
52 files changed, 117 insertions(+), 111 deletions(-)
diff --git a/bisect.c b/bisect.c
index e1275ba79e..41c56a665e 100644
--- a/bisect.c
+++ b/bisect.c
@@ -807,7 +807,7 @@ static void check_merge_bases(int rev_nr, struct commit
**rev, int no_checkout)
for (; result; result = result->next) {
const struct object_id *mb = >item->object.oid;
- if (!oidcmp(mb, current_bad_oid)) {
+ if (oideq(mb, current_bad_oid)) {
handle_bad_merge_base();
} else if (0 <= oid_array_lookup(_revs, mb)) {
continue;
@@ -988,7 +988,7 @@ int bisect_next_all(const char *prefix, int no_checkout)
bisect_rev = >item->object.oid;
- if (!oidcmp(bisect_rev, current_bad_oid)) {
+ if (oideq(bisect_rev, current_bad_oid)) {
exit_if_skipped_commits(tried, current_bad_oid);
printf("%s is the first %s commit\n", oid_to_hex(bisect_rev),
term_bad);
diff --git a/blame.c b/blame.c
index 08c0c6cf73..10d72e36dd 100644
--- a/blame.c
+++ b/blame.c
@@ -1459,14 +1459,14 @@ static void pass_blame(struct blame_scoreboard *sb,
struct blame_origin *origin,
porigin = find(p, origin);
if (!porigin)
continue;
- if (!oidcmp(>blob_oid, >blob_oid)) {
+ if (oideq(>blob_oid, >blob_oid)) {
