| Message ID | 54F0DF2F.9020404@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Feb 27, 2015, "Carlos O'Donell" <carlos@redhat.com> 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.
Anyway, if you really feel like playing against your stated goal of
making patches to this area easier to apply before and after (which I
DID take into account when deciding how to make the change to begin
with, mind you, because I share that concern), I won't stand in the way.
The difference is only one or two lines in the surface area anyway, so
go ahead with whatever you think is best.
On 02/27/2015 04:34 PM, Alexandre Oliva wrote: > On Feb 27, 2015, "Carlos O'Donell" <carlos@redhat.com> 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? Cheers, Carlos.
"Carlos O'Donell" <carlos@redhat.com> writes:
> Could you please checkin your original solution as-is?
Why? It makes the code less readable by using obscure variable names.
Andreas.
On 02/28/2015 02:23 AM, Andreas Schwab wrote: > "Carlos O'Donell" <carlos@redhat.com> writes: > >> Could you please checkin your original solution as-is? > > Why? It makes the code less readable by using obscure variable names. Please feel free to rename cloc_name to something else. I have withdrawn my objections given Alex's explanations. Cheers, Carlos.
diff --git a/locale/findlocale.c b/locale/findlocale.c index 5e2639b..2e98c31 100644 --- a/locale/findlocale.c +++ b/locale/findlocale.c @@ -105,7 +105,8 @@ _nl_find_locale (const char *locale_path, size_t locale_path_len, { int mask; /* Name of the locale for this category. */ - char *loc_name = (char *) *name; + const char *loc_name = *name; + char *loc_name_copy; const char *language; const char *modifier; const char *territory; @@ -124,7 +125,7 @@ _nl_find_locale (const char *locale_path, size_t locale_path_len, if (!name_present (loc_name)) loc_name = getenv ("LANG"); if (!name_present (loc_name)) - loc_name = (char *) _nl_C_name; + loc_name = _nl_C_name; } /* We used to fall back to the C locale if the name contains a slash @@ -136,7 +137,7 @@ _nl_find_locale (const char *locale_path, size_t locale_path_len, { /* We need not load anything. The needed data is contained in the library itself. */ - *name = (char *) _nl_C_name; + *name = _nl_C_name; return _nl_C[category]; } else if (!valid_locale_name (loc_name)) @@ -158,11 +159,10 @@ _nl_find_locale (const char *locale_path, size_t locale_path_len, /* Nothing in the archive with the given name. Expanding it as an alias and retry. */ - loc_name = (char *) _nl_expand_alias (*name); + loc_name = _nl_expand_alias (*name); if (loc_name != NULL) { - data = _nl_load_locale_from_archive (category, - (const char **) &loc_name); + data = _nl_load_locale_from_archive (category, &loc_name); if (__builtin_expect (data != NULL, 1)) return data; } @@ -175,14 +175,14 @@ _nl_find_locale (const char *locale_path, size_t locale_path_len, /* We really have to load some data. First see whether the name is an alias. Please note that this makes it impossible to have "C" or "POSIX" as aliases. */ - loc_name = (char *) _nl_expand_alias (*name); + loc_name = _nl_expand_alias (*name); if (loc_name == NULL) /* It is no alias. */ - loc_name = (char *) *name; + loc_name = *name; /* Make a writable copy of the locale name. */ - loc_name = strdupa (loc_name); + loc_name_copy = strdupa (loc_name); /* LOCALE can consist of up to four recognized parts for the XPG syntax: @@ -197,7 +197,7 @@ _nl_find_locale (const char *locale_path, size_t locale_path_len, (3) territory (4) modifier */ - mask = _nl_explode_name (loc_name, &language, &modifier, &territory, + mask = _nl_explode_name (loc_name_copy, &language, &modifier, &territory, &codeset, &normalized_codeset); if (mask == -1) /* Memory allocate problem. */