This is the mail archive of the mailing list for the glibc project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: search locale archive again after alias expansion

On 02/27/2015 04:34 PM, Alexandre Oliva wrote:
> On Feb 27, 2015, "Carlos O'Donell" <> wrote:
>> That is ~9 lines of changes vs. the original ~22 lines of change.
> Yeah, it is smaller, but it still covers a larger code area and thus
> *more* lines of context, that will make patches in this area just as
> hard, if not harder, to apply cleanly.  Plus, by not moving the
> declaration down and changing the type of a variable, patches that apply
> to the now-const area and introduce further references to loc_name might
> still seem to apply with minor adjustments, even though they might
> require further adjustments to account for the type change.

That is probably the best argument for your patch and should have been
right up there with the original patch submission.

I think the key point is that your patch *appears* to be larger than
required, and that raises a red flag for me. Then I have to go digging
to determine why you arrived at your solution. You didn't make it easy
for me, the grader, to give you an A.

I see now that your original patch was 4 hunks vs my 6, covering a smaller
area of code. It also reduces the likelihood of adding back non-const
uses of loc_name during backpors (by renaming to cloc_name) which is
important. It therefore seems like the best balance of both changes.

I'm sorry for the back and forth.

Could you please checkin your original solution as-is?


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]