diff mbox series

[2/2] object.c: initialize automatic variable in lookup_object()

Message ID patch-2.3-f1fcc31717-20210409T080534Z-avarab@gmail.com (mailing list archive)
State New
Headers show
Series blob/object.c: trivial readability improvements | expand

Commit Message

Ævar Arnfjörð Bjarmason April 9, 2021, 8:07 a.m. UTC
Initialize a "struct object obj*" variable to NULL explicitly and
return it instead of leaving it uninitialized until the "while"
loop.

There was no bug here, it's just less confusing when debugging if the
"obj" is either NULL or a valid object, not some random invalid
pointer.

See 0556a11a0df (git object hash cleanups, 2006-06-30) for the initial
implementation.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 object.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Jeff King April 9, 2021, 5:53 p.m. UTC | #1
On Fri, Apr 09, 2021 at 10:07:28AM +0200, Ævar Arnfjörð Bjarmason wrote:

> Initialize a "struct object obj*" variable to NULL explicitly and
> return it instead of leaving it uninitialized until the "while"
> loop.
> 
> There was no bug here, it's just less confusing when debugging if the
> "obj" is either NULL or a valid object, not some random invalid
> pointer.
>
> [...]
>
>  struct object *lookup_object(struct repository *r, const struct object_id *oid)
>  {
>  	unsigned int i, first;
> -	struct object *obj;
> +	struct object *obj = NULL;
>  
>  	if (!r->parsed_objects->obj_hash)
> -		return NULL;
> +		return obj;

I actually prefer the original style (where any "can we bail early"
checks just explicitly return NULL, rather than making you check to see
that obj is NULL). But it's pretty subjective, and I don't feel
strongly.

>  	first = i = hash_obj(oid, r->parsed_objects->obj_hash_size);
>  	while ((obj = r->parsed_objects->obj_hash[i]) != NULL) {

The important thing is that "obj" is not used uninitialized, which it
isn't (before or after).

-Peff
Junio C Hamano April 9, 2021, 10:32 p.m. UTC | #2
Jeff King <peff@peff.net> writes:

>>  struct object *lookup_object(struct repository *r, const struct object_id *oid)
>>  {
>>  	unsigned int i, first;
>> -	struct object *obj;
>> +	struct object *obj = NULL;
>>  
>>  	if (!r->parsed_objects->obj_hash)
>> -		return NULL;
>> +		return obj;
>
> I actually prefer the original style (where any "can we bail early"
> checks just explicitly return NULL, rather than making you check to see
> that obj is NULL).

Perhaps I should return "me too" here.
diff mbox series

Patch

diff --git a/object.c b/object.c
index 63896abf01..7fdca3ed1e 100644
--- a/object.c
+++ b/object.c
@@ -87,10 +87,10 @@  static void insert_obj_hash(struct object *obj, struct object **hash, unsigned i
 struct object *lookup_object(struct repository *r, const struct object_id *oid)
 {
 	unsigned int i, first;
-	struct object *obj;
+	struct object *obj = NULL;
 
 	if (!r->parsed_objects->obj_hash)
-		return NULL;
+		return obj;
 
 	first = i = hash_obj(oid, r->parsed_objects->obj_hash_size);
 	while ((obj = r->parsed_objects->obj_hash[i]) != NULL) {