Hey Matthieu,
Sorry for the late reply. Somehow your email didn't receive my
mailbox. I got to see this mail when I was going through the gmane
archives.
Matthieu Moy <Matthieu.Moy <at> grenoble-inp.fr> writes:
Pranit Bauva <pranit.bauva <at> gmail.com> writes:
>> +int bisect_voc(const char *term)
>> +{
>> + if (!strcmp(term, "bad"))
>> + printf("bad|new\n");
>> + if (!strcmp(term, "good"))
>> + printf("good|old\n");
>
> If you meant to use this as a helper command, then the implementation is
> right, but you're not doing that.
> If you write the function because one day you'll be calling it from C,
> then:
> 1) First, I'd wait for this "one day" to happen. In general, write code
> when you need it, don't write it ahead of time. Currently, you have
> dead and untested code (I know, *you* have tested it, but it's still
> "untested" as far as git.git is concerned). Dead code may bother
> people reading the code (one would not understand why it's there),
> and untested code means it may break later without anyone noticing.
>
I think this function can wait then. I will include this patch when
its really required. I wanted to convert this function ASAP because it
was a really tiny one and an easy one.
> 2) Second, you'd need to return the string, not print it. You'll
> typically use it like this:
> printf(_("You need to give me at least one %s and one %s"),
> bisect_voc(BISECT_BAD), bisect_voc(BISECT_GOOD));
> which gives one more argument for 1): once you have a use-case, you
> can design the API properly, and not blindly guess that you're going
> to need printf. Actually, writting these 2 example lines, I also
> noticed that the parameters could/should be an enum type rather than
> a string, it makes the code both more efficient and clearer.
>
Okay I get it. It would be much more efficient to return a enum
because its difficult to parse text output into a C program. I hadn't
looked further into the function. Thanks for pointing it out early!
I will wait before re-rolling this patch and will do it when I convert
bisect_state().
Regards,
Pranit Bauva
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html