Re: [PATCH] Code cleanup in load_module

[Tim Rühsen]

On 4/8/20 6:37 PM, Jose E. Marchesi wrote:
> Hi Tim.
>
>      2020-04-08  Tim Rühsen  <address@hidden>
>
>              * lib/pkl-tab.y (load_module): Code cleanup.
>      ---
>       ChangeLog     |  4 ++++
>       lib/pkl-tab.y | 46 ++++++++++++++++------------------------------
>       2 files changed, 20 insertions(+), 30 deletions(-)
>
>      diff --git a/lib/pkl-tab.y b/lib/pkl-tab.y
>      index 6a8f17f5..22708f46 100644
>      --- a/lib/pkl-tab.y
>      +++ b/lib/pkl-tab.y
>      @@ -224,41 +224,27 @@ load_module (struct pkl_parser *parser,
>            requested module.  */
>         {
>           char *full_filename;
>      -    char *saveptr = NULL;
>      -    char *path = xstrdup (load_path); /* Modified by strtok.  */
>      -    char *dir = strtok_r (path, ":", &saveptr);
>      +    const char *s, *e;
>
>      -    if (dir)
>      +    for (s = load_path, e = s; *e; s = e + 1)
>             {
>      -        do
>      +        /* Ignore empty entries. */
>      +        if ((e = strchrnul (s, ':')) == s)
>      +          continue;
>      +
>      +        if (!strncmp (s, "%DATADIR%", e - s))
>      +          full_filename = pk_str_concat (PKGDATADIR, "/", filename, NULL);
>
> Only now I realize that we are not doing the right thing for situations
> where %DATADIR% has some trailing stuff before the next ':' or '\0',
> like in:
>
>     :%DATADIR%foo:
>
> If I'm not wrong in both the previous implementation and this new one
> `foo' would be ignored.

Right.

> We could not do the replacement in that case, but it would be nice to
> support something like:
>
>    :%DATADIR%/mysubdir:
>
> What about something like:
>
>
> if (!strncmp (s, "%DATADIR%", e - s))
>     full_filename = pk_str_concat (PKGDATADIR, "/", filename,
>                                    s + strlen ("%DATADIR%"),
>                                    NULL);

That makes sense. I'll update the patch and will look for other similar
patterns as well (possibly I saw this somewhere else).