search locale archive again after alias expansion
diff mbox

Message ID 54F0DF2F.9020404@redhat.com
State New
Headers show

Commit Message

Carlos O'Donell Feb. 27, 2015, 9:18 p.m. UTC
On 02/27/2015 04:03 PM, Alexandre Oliva wrote:
> On Feb 27, 2015, "Carlos O'Donell" <carlos@redhat.com> wrote:
> 
>> Is that minimal? Does that solve the casting issue?
> 
> Of course not.  Plenty of casts that drop const, now superfluous, remain
> in place with your patch.  The point of the patch was to remove them
> all.

Now?

That is ~9 lines of changes vs. the original ~22 lines of change.
 
---

Cheers,
Carlos.

Comments

Alexandre Oliva Feb. 27, 2015, 9:34 p.m. UTC | #1
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.
Carlos O'Donell Feb. 27, 2015, 9:57 p.m. UTC | #2
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.
Andreas Schwab Feb. 28, 2015, 7:23 a.m. UTC | #3
"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.
Carlos O'Donell Feb. 28, 2015, 5:39 p.m. UTC | #4
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.

Patch
diff mbox

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.  */