On Sun, Aug 9, 2015 at 9:12 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
> On Sat, Aug 8, 2015 at 2:35 AM, Karthik Nayak <karthik.188@xxxxxxxxx> wrote:
>> On Fri, Aug 7, 2015 at 8:57 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
>>> On Wed, Aug 5, 2015 at 2:54 PM, Karthik Nayak <karthik.188@xxxxxxxxx> wrote:
>>>> of the padding to be performed. If the atom length is more than the
>>>> padding length then no padding is performed. e.g. to pad a succeeding
>>>> atom to the middle with a total padding size of 40 we can do a
>>>
>>> It's odd to have alignment described in terms of "padding" and
>>> "padding length", especially in the case of "center" alignment. It
>>> might be better to rephrase the discussion in terms of field width or
>>> such.
>>>
>>>> --format="%(align:middle,40).."
>>
>> Ok this makes sense,
>> I'll rephrase as :
>>
>> `<width>` is the total length of the content with alignment.
>
> This doesn't really make sense. <width> isn't the content length; it's
> the size of the area into which the content will be placed.
>
Will change this.
>> If the atom length is more than the width then no alignment is performed.
>
> What "atom"? I think you mean the content between %(align:) and %(end)
> rather than "atom". The description might be clearer if you actually
> say "content between %(align:) and %(end)" to indicate specifically
> what is being aligned.
Yes, that's what I meant.
>
>> e.g. to align a succeeding atom to the middle with a total width of 40
>> we can do:
>> --format="%(align:middle,40).."
>>>> @@ -687,6 +690,29 @@ static void populate_value(struct ref_array_item *ref)
>>>> else
>>>> v->s = " ";
>>>> continue;
>>>> + } else if (starts_with(name, "align:")) {
>>>> + const char *valp = NULL;
>>>
>>> Unnecessary NULL assignment.
>>
>> Thats required for the second skip_prefix and so on.
>> Else we get: "warning: ‘valp’ may be used uninitialized in this
>> function [-Wmaybe-uninitialized]"
>
> Okay, so that's because skip_prefix() is inline, and it doesn't touch
> its "out" argument unless it actually skips the prefix. Ugly, but
> makes sense, although I think this issue would go away if you combined
> the starts_with() and skips_prefix() as suggested earlier.
>
Okay then I'll declare valp prehand to get rid of this and also to
remove redundant, starts_with() and skip_prefix().
>>>> + struct align *align = xmalloc(sizeof(struct align));
>>>> +
>>>> + skip_prefix(name, "align:", &valp);
>>>
>>> You could simplify the code by combining this skip_prefix() with the
>>> earlier starts_with() in the conditional:
>>>
>>> } else if (skip_prefix(name, "align:", &valp)) {
>>> struct align *align = xmalloc(sizeof(struct align));
>>> ...
>>
>> That would require valp to be previously defined. Hence the split.
>
> Yes, it would require declaring 'valp' earlier, but that seems a
> reasonable tradeoff for cleaner, simpler, less redundant code.
>
Yes. will do this.
>>>> static void apply_formatting_state(struct ref_formatting_state *state, struct strbuf *final)
>>>> {
>>>> - /* More formatting options to be evetually added */
>>>> + if (state->align && state->end) {
>>>> + struct strbuf *value = state->output;
>>>> + int len = 0, buf_len = value->len;
>>>> + struct align *align = state->align;
>>>> +
>>>> + if (!value->buf)
>>>> + return;
>>>> + if (!is_utf8(value->buf)) {
>>>> + len = value->len - utf8_strwidth(value->buf);
>>>
>>> What is this doing, exactly? If the string is *not* utf-8, then you're
>>> asking it for its utf-8 width. Am I reading that correctly?
>>>
>>> Also, what is 'len' supposed to represent? I guess you want it to be
>>> the difference between the byte length and the display length, but the
>>> name 'len' doesn't convey that at all, nor does it help the reader
>>> understand the code below where you do the actual formatting.
>>>
>>> In fact, if I'm reading this correctly, then 'len' is always zero in
>>> your tests (because the tests never trigger this conditional), so this
>>> functionality is never being exercised.
>>
>> There shouldn't be a "!" there, will change.
>> I guess 'utf8_compensation' would be a better name.
>
> Definitely better than 'len'.
>
>>>> + else if (align->align_type == ALIGN_MIDDLE) {
>>>> + int right = (align->align_value - buf_len)/2;
>>>> + strbuf_addf(final, "%*s%-*s", align->align_value - right + len,
>>>> + value->buf, right, "");
>>>
>>> An aesthetic aside: When (align_value - buf_len) is an odd number,
>>> this implementation favors placing more whitespace to the left of the
>>> string, and less to the right. In practice, this often tends to look a
>>> bit more awkward than the inverse of placing more whitespace to the
>>> right, and less to the left (but that again is subjective).
>>
>> I know that, maybe we could add an additional padding to even out the value
>> given?
>
> I don't understand your question. I was merely suggesting (purely
> subjectively), for the "odd length" case, putting the extra space
> after the centered text rather than before it. For instance:
>
> int left = (align->align_value - buf_len) / 2;
> strbuf_addf(final, "%*s%-*s", left, "",
> align->align_value - left + len, value->buf);
>
> or any similar variation which would give the same result.
>
I get this could be done, what I was asking was, Consider given a alignment
width of 25 would be better to make that 26 so that we have even padding on
both sides. But I don't like the adding of manipulating user given data.
>>> This is a tangent, but I could easily see all of the code from 'if
>>> (align->align_value < buf_len)' down to this point being placed in
>>> utf8.c as a general alignment utility function. Doing so would make
>>> this function shorter, and the patch easier to review overall (which
>>> might be an important consideration -- especially given that I've
>>> already spent several hours reviewing this one patch).
>>
>> That's a valid suggestion, will do that, thanks, but that'd mean we need to
>> send an align struct to utf8.c which is only defined in ref-filter.h.
>> Either this
>> is fine or we need to move the definition of struct align to utf8.h.
>> I think personally move the align structure and enum over to utf8.h
>
> No, you don't need to move the 'struct align' to utf8.h. That
> structure is specific to ref-filter and should stay there. Instead,
> you only need to move the enum. For instance, you'd add something like
> this to utf8.h:
>
> enum utf8_alignment {
> ALIGN_LEFT,
> ALIGN_MIDDLE,
> ALIGN_RIGHT
> };
>
> void strbuf_utf8_align(struct strbuf *buf,
> utf8_alignment where, int width, const char *s);
>
Okay will do this.
> By the way, I forgot to say earlier that this should be done as a
> separate patch (in order to make the current patch smaller).
>
Of course, that was obvious :)
> That raises another question. Why are 'struct ref_formatting_state',
> 'struct align', 'struct atom_value', etc. defined in ref-filter.h at
> all? Aren't those private implementation details of ref-filter.c, or
> do you expect other code to be using them?
>
I guess struct ref_formatting_state and struct align could be moved to
ref-filter.c. About struct atom_value its referenced by ref_array_item()
so any reader reading about this, would find it easier if atom_value()
is at the same place.
>>>> for (i = 0; i < final_buf.len; i++)
>>>> printf("%c", final_buf.buf[i]);
>>>> putchar('\n');
>>>> diff --git a/ref-filter.h b/ref-filter.h
>>>> index 9e6c2d4..5575fe9 100644
>>>> --- a/ref-filter.h
>>>> +++ b/ref-filter.h
>>>> @@ -16,14 +16,30 @@
>>>> struct ref_formatting_state {
>>>> - int quote_style;
>>>> struct strbuf *output;
>>>> + struct align *align;
>>>> + int quote_style;
>>>
>>> Perhaps decide where you'd like 'quote_style' to reside from the start
>>> so that you don't have to add it at one location in its introductory
>>> patch and then move it in a later patch. Also, why move it here?
>>
>> Cause that'd save memory on a 64 bit processor, where the pointers would
>> be 8 bytes long and int would be 4 bytes long, this would bring in padding if
>> int is placed before the pointers. Will change before hand.
>
> As I understand it, you're not likely to have many
> ref_fomratting_state's around at any given time, so this sounds like
> premature memory micro-optimization.
Agreed, its a micro-optimization, but why leave it out when we only need
to re-structure code? I'll probably change it beforehand.
--
Regards,
Karthik Nayak
--
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