Re: [PATCH v8 7/8] cache: add empty_tree_oid object

[Junio C Hamano]

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

[Jacob Keller]

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]

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

[Jeff King]

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

[Junio C Hamano]

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

[Jacob Keller]

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

[Junio C Hamano]

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

[Jacob Keller]

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