Re: [PATCH v15 08/27] bisect--helper: `is_expected_rev` & `check_expected_revs` shell function in C

From: Stephan Beyer <s-beyer@gmx.net>
Date: Dec 17, 2016, 19:42 UTC
Message-ID: <22624028-b22d-5e4f-7101-c97e6e3e853c@gmx.net>
In-Reply-To: <CAFZEwPO2WgBjOnmvu1VOiz3PMYYx2mxircCWk+BWxmuunC=VQA@mail.gmail.com>

Hi Pranit,

On 12/16/2016 08:00 PM, Pranit Bauva wrote:

Show 14 quoted lines

> On Wed, Dec 7, 2016 at 1:03 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>> I don't understand why the return value is int and not void. To avoid a
>>> "return 0;" line when calling this function?
>>
>> Initially I thought I would be using the return value but now I
>> realize that it is meaningless to do so. Using void seems better. :)
> 
> I just recollected when I was creating the next iteration of this
> series that I will need that int.
> 
>>>> +     case CHECK_EXPECTED_REVS:
>>>> +             return check_expected_revs(argv, argc);
> 
> See this.

This does not show that you need the "int", it just shows that you use the return value of the function. But this return value is (in the original shell code as well as in your v15) always 0. That is a sign that the "void" return value makes more sense. Of course, then the line above must be changed to

+     case CHECK_EXPECTED_REVS:
+             check_expected_revs(argv, argc);
+             return 0;

By the way, it also seems that the original function name "check_expected_revs" was not the best choice (because the function always returns 0, but the "check" implies that some semantically true or false is returned). On the other hand, it might also be useful (I cannot tell) to return different values in check_expected_revs() — to signal the caller that something changed or something did not change — but then ignore the return value.

Best
  Stephan