Re: [PATCH v8 7/8] cache: add empty_tree_oid object
Jeff King writes: > I suspect we need more than just the "is_empty" query. At least for the > blob case, we do hashcpy() it into a struct (which should eventually > become oidcpy). The empty-tree case even more so, as we pass it to > random functions like lookup_tree(). > > Our EMPTY_TREE_SHA1_BIN_LITERAL effectively ends up as a singleton > in-core, too; it's just handled transparently by the compiler, since > it's a literal. This effectively gives us _two_ singletons. We could do: > > const struct object_id empty_blob_oid = { > "\xe6\x9d\xe2\x9b\xb2\xd1\xd6\x43\x4b\x8b" > "\x29\xae\x77\x5a\xd8\xc2\xe4\x8c\x53\x91" > }; > #define EMPTY_BLOB_SHA1_BIN (empty_blob_oid.hash) > > It's possible the use of an actual string literal lets the compiler do > more optimizations, but I'd doubt it matters in practice. Probably it is > just sticking that literal somewhere in BSS and filling in the pointer > to it. Makes sense; thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 7/8] cache: add empty_tree_oid object
On Fri, Aug 19, 2016 at 2:14 PM, Junio C Hamano wrote: > Junio C Hamano writes: > >> Jacob Keller writes: >> >>> Is there a reason for that? I've found that .field = value is safer >>> because it ensures that you don't end up initializing the wrong >>> values? Or is it a compatibility thing? >> >> Yes. > > That was a bit too terse. The answers to all three questions are > "Yes", "Yes", and "Yes". > > Together with enum ... = { A, B, C, }; we may want to consider not > worrying about ancient compilers at some point, and it might be this > year or next year, but I do not think we want to do that as part of > this series. Thanks, it makes sense. It is hard to remember when some feature got added. Regards, Jake -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 7/8] cache: add empty_tree_oid object
Junio C Hamano writes: > Jacob Keller writes: > >> Is there a reason for that? I've found that .field = value is safer >> because it ensures that you don't end up initializing the wrong >> values? Or is it a compatibility thing? > > Yes. That was a bit too terse. The answers to all three questions are "Yes", "Yes", and "Yes". Together with enum ... = { A, B, C, }; we may want to consider not worrying about ancient compilers at some point, and it might be this year or next year, but I do not think we want to do that as part of this series. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 7/8] cache: add empty_tree_oid object
On Fri, Aug 19, 2016 at 01:31:27PM -0700, Junio C Hamano wrote: > Jacob Keller writes: > > > From: Jacob Keller > > > > Add an empty_tree_oid object which can be used in place of > > EMPTY_TREE_SHA1_BIN_LITERAL for code which is being converted to struct > > object_id. > > How widely do you envision the users of this symbol would be spread > across the entire codebase? I am debating myself if we need a > singleton in-core copy like this (if we end up referring to it from > everywhere), or we only need one level higher abstraction, > e.g. "is_empty_tree_oid()" helper (in which case I do not think we > even need a singleton; just imitate how is_empty_blob_sha1() is > implemented). I suspect we need more than just the "is_empty" query. At least for the blob case, we do hashcpy() it into a struct (which should eventually become oidcpy). The empty-tree case even more so, as we pass it to random functions like lookup_tree(). Our EMPTY_TREE_SHA1_BIN_LITERAL effectively ends up as a singleton in-core, too; it's just handled transparently by the compiler, since it's a literal. This effectively gives us _two_ singletons. We could do: const struct object_id empty_blob_oid = { "\xe6\x9d\xe2\x9b\xb2\xd1\xd6\x43\x4b\x8b" "\x29\xae\x77\x5a\xd8\xc2\xe4\x8c\x53\x91" }; #define EMPTY_BLOB_SHA1_BIN (empty_blob_oid.hash) It's possible the use of an actual string literal lets the compiler do more optimizations, but I'd doubt it matters in practice. Probably it is just sticking that literal somewhere in BSS and filling in the pointer to it. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 7/8] cache: add empty_tree_oid object
Jacob Keller writes: > Is there a reason for that? I've found that .field = value is safer > because it ensures that you don't end up initializing the wrong > values? Or is it a compatibility thing? Yes. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 7/8] cache: add empty_tree_oid object
On Fri, Aug 19, 2016 at 1:31 PM, Junio C Hamano wrote: > Jacob Keller writes: > >> From: Jacob Keller >> >> Add an empty_tree_oid object which can be used in place of >> EMPTY_TREE_SHA1_BIN_LITERAL for code which is being converted to struct >> object_id. > > How widely do you envision the users of this symbol would be spread > across the entire codebase? I am debating myself if we need a > singleton in-core copy like this (if we end up referring to it from > everywhere), or we only need one level higher abstraction, > e.g. "is_empty_tree_oid()" helper (in which case I do not think we > even need a singleton; just imitate how is_empty_blob_sha1() is > implemented). If I do this, I'd also add an "is_empty_tree_sha1()" as well? > > Even if we need such a singleton, I think we avoid ".field = value" > struct initializations in our code. > Is there a reason for that? I've found that .field = value is safer because it ensures that you don't end up initializing the wrong values? Or is it a compatibility thing? Thanks, Jake -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 7/8] cache: add empty_tree_oid object
Jacob Keller writes: > From: Jacob Keller > > Add an empty_tree_oid object which can be used in place of > EMPTY_TREE_SHA1_BIN_LITERAL for code which is being converted to struct > object_id. How widely do you envision the users of this symbol would be spread across the entire codebase? I am debating myself if we need a singleton in-core copy like this (if we end up referring to it from everywhere), or we only need one level higher abstraction, e.g. "is_empty_tree_oid()" helper (in which case I do not think we even need a singleton; just imitate how is_empty_blob_sha1() is implemented). Even if we need such a singleton, I think we avoid ".field = value" struct initializations in our code. > > Signed-off-by: Jacob Keller > --- > cache.h | 2 ++ > sha1_file.c | 3 +++ > 2 files changed, 5 insertions(+) > > diff --git a/cache.h b/cache.h > index f30a4417efdf..da9f0be67d7b 100644 > --- a/cache.h > +++ b/cache.h > @@ -964,6 +964,8 @@ static inline void oidclr(struct object_id *oid) > #define EMPTY_BLOB_SHA1_BIN \ > ((const unsigned char *) EMPTY_BLOB_SHA1_BIN_LITERAL) > > +extern const struct object_id empty_tree_oid; > + > static inline int is_empty_blob_sha1(const unsigned char *sha1) > { > return !hashcmp(sha1, EMPTY_BLOB_SHA1_BIN); > diff --git a/sha1_file.c b/sha1_file.c > index 1e23fc186a02..10883d56a600 100644 > --- a/sha1_file.c > +++ b/sha1_file.c > @@ -38,6 +38,9 @@ static inline uintmax_t sz_fmt(size_t s) { return s; } > > const unsigned char null_sha1[20]; > const struct object_id null_oid; > +const struct object_id empty_tree_oid = { > + .hash = EMPTY_TREE_SHA1_BIN_LITERAL > +}; > > /* > * This is meant to hold a *small* number of objects that you would -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v8 7/8] cache: add empty_tree_oid object
From: Jacob Keller Add an empty_tree_oid object which can be used in place of EMPTY_TREE_SHA1_BIN_LITERAL for code which is being converted to struct object_id. Signed-off-by: Jacob Keller --- cache.h | 2 ++ sha1_file.c | 3 +++ 2 files changed, 5 insertions(+) diff --git a/cache.h b/cache.h index f30a4417efdf..da9f0be67d7b 100644 --- a/cache.h +++ b/cache.h @@ -964,6 +964,8 @@ static inline void oidclr(struct object_id *oid) #define EMPTY_BLOB_SHA1_BIN \ ((const unsigned char *) EMPTY_BLOB_SHA1_BIN_LITERAL) +extern const struct object_id empty_tree_oid; + static inline int is_empty_blob_sha1(const unsigned char *sha1) { return !hashcmp(sha1, EMPTY_BLOB_SHA1_BIN); diff --git a/sha1_file.c b/sha1_file.c index 1e23fc186a02..10883d56a600 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -38,6 +38,9 @@ static inline uintmax_t sz_fmt(size_t s) { return s; } const unsigned char null_sha1[20]; const struct object_id null_oid; +const struct object_id empty_tree_oid = { + .hash = EMPTY_TREE_SHA1_BIN_LITERAL +}; /* * This is meant to hold a *small* number of objects that you would -- 2.10.0.rc0.217.g609f9e8.dirty -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html