Re: bin/41700: :tr modifier for NetBSD make [patch]

[Aleksey Cheusov <vle%gmx.net@localhost>]

I move this discussion to the mailing list.

>  Some comments about the code....
>  (NB not about whether this is a good idea)
>  
 >> +static char *
 >> +VarTranslate(char *str, char *chars, char *repl)
 >> +{
 >> +   Buffer  buf;
 >> +   int     table [UCHAR_MAX+1];
 >> +   int     r;
 >> +   int     c;
 >> +
 >> +   /* initialization of translation table */
 >> +   memset (table, -1, sizeof (table));
>  
>  initialize table[n] = n here, table can then be char.

Ok.

 >> +   for (; *chars; ++chars, ++repl){
 >> +       assert (*repl != 0);
>  
>  Don't assert! I can't remember what tr(1) does, but terminating
>  the action wouldn't be too bad.
>  
 >> +       table [(unsigned char) *chars] = (unsigned char) *repl;
>  
>  You need to support ranges!

Do others also think ranges are really necessary?  I my view this rarely
used feature (:tu and :tl are already implemented) adds unnecessary
complexity to the code and makes bmake Makefiles dependent on ASCII
(remember EBCDIC). Inventing character classes for :tr adds yet another
unnecessary bloat...  I'd prefer not to add ranges.

 >> +   }
 >> +   assert (*repl == 0);
 >> +
 >> +   /* translation */
>  
>  Don't use 'buf', just tgt = malloc(strlen(str) + 1)

Ok.

 >> +   Buf_Init(&buf, 0);
 >> +   for (; *str ; ++str) {
 >> +       c = (unsigned char) *str;
 >> +       r = table [c];
 >> +       if (r == -1)
 >> +      Buf_AddByte(&buf, (Byte)c);
 >> +       else
 >> +      Buf_AddByte(&buf, (Byte)r);
 >> +   }
 >> +   Buf_AddByte(&buf, (Byte)'\0');
>  In any case Buf_Addbyte guarantees \0 terminated

Ok.

 >> +   str = (char *)Buf_GetAll(&buf, NULL);
 >> +   Buf_Destroy(&buf, FALSE);
 >> +   return str;
 >> +}
 >> +
 >> +/*-
 >> + *-----------------------------------------------------------------------
 >>   * VarChangeCase --
 >>   *      Change the string to all uppercase or all lowercase
 >>   *
 >> @@ -2655,7 +2707,47 @@
 >>     case 't':
 >>         {
 >>             cp = tstr + 1;  /* make sure it is set */
 >> -           if (tstr[1] != endc && tstr[1] != ':') {
 >> +           if (tstr[1] == 'r') {
 >> +               char *chars, *repl;
 >> +               int chars_len, repl_len;
 >> +
 >> +               delim = tstr[2];
 >> +               tstr += 3;
 >> +
 >> +               cp = tstr;
 >> +               chars = VarGetPattern(
 >> +                   ctxt, &parsestate, errnum,
 >> +                   &cp, delim, NULL,
 >> +                   &chars_len,
 >> +                   NULL);
 >> +               if (!chars)
 >> +                   goto cleanup;
 >> +
 >> +               repl = VarGetPattern(
 >> +                   ctxt, &parsestate, errnum,
 >> +                   &cp, delim, NULL,
 >> +                   &repl_len,
 >> +                   NULL);
 >> +               if (!repl)
 >> +                   goto cleanup;
 >> +
 >> +               delim = '\0';
>  
>  No need to do the check below twice!

Do you mean assert(3)s?

 >> +               if (strlen (chars) != strlen (repl)){
 >> +                   free(chars);
 >> +                   free(repl);
 >> +
 >> +                   Error("Bad arguments for :tr modifier for variable %s",
 >> +                         v->name);
 >> +                   goto cleanup;
 >> +               }
 >> +
 >> +               termc = *cp;
 >> +               newStr = VarTranslate(nstr, chars, repl);
 >> +
 >> +               free(chars);
 >> +               free(repl);
 >> +           }else if (tstr[1] != endc && tstr[1] != ':') {
 >>                 if (tstr[1] == 's') {
 >>                     /*
 >>                      * Use the char (if any) at tstr[2]
 >> 

-- 
Best regards, Aleksey Cheusov.