On Tue, Jun 09, 2020 at 11:46:24AM +0200, Pavel Stehule wrote:
> po 8. 6. 2020 v 23:30 odesÃlatel Justin Pryzby <[email protected]> napsal:
> I still wonder if a better syntax would use a unified --filter option, whose
> > argument would allow including/excluding any type of object:
> I tried to implement simple format "[+-][tndf] objectname"
Thanks.
> + /* ignore empty rows */
> + if (*line != '\0')
Maybe: if line=='\0': continue
We should also support comments.
> + bool
> include_filter = false;
> + bool
> exclude_filter = false;
I think we only need one bool.
You could call it: bool is_exclude = false
> +
> + if (chars < 4)
> +
> invalid_filter_format(optarg, line, lineno);
I think that check is too lax.
I think it's ok if we require the first char to be [-+] and the 2nd char to be
[dntf]
> + objecttype = line[1];
... but I think this is inadequately "liberal in what it accepts"; I think it
should skip spaces. In my proposed scheme, someone might reasonably write:
> +
> + objectname = &line[3];
> +
> + /* skip initial spaces
> */
> + while (*objectname == '
> ')
> + objectname++;
I suggest to use isspace()
I think we should check that *objectname != '\0', rather than chars>=4, above.
> + if
> (include_filter)
> + {
> +
> simple_string_list_append(&table_include_patterns, objectname);
> +
> dopt.include_everything = false;
> + }
> + else if
> (exclude_filter)
> +
> simple_string_list_append(&table_exclude_patterns, objectname);
If you use bool is_exclude, then this becomes "else" and you don't need to
think about checking if (!include && !exclude).
> + else if (objecttype ==
> 'f')
> + {
> + if
> (include_filter)
> +
> simple_string_list_append(&foreign_servers_include_patterns, objectname);
> + else if
> (exclude_filter)
> +
> invalid_filter_format(optarg, line, lineno);
> + }
I would handle invalid object types as "else: invalid_filter_format()" here,
rather than duplicating above as: !=ALL('d','n','t','f')
> +
> + if (ferror(f))
> + fatal("could not read from file
> \"%s\": %s",
> + f == stdin ? "stdin"
> : optarg,
> + strerror(errno));
I think we're allowed to use %m here ?
> + printf(_(" --filter=FILENAME read object names from
> file\n"));
Object name filter expression, or something..
> + * getline is originaly GNU function, and should not be everywhere still.
originally
> + * Use own reduced implementation.
Did you "reduce" this from another implementation? Where?
What is its license ?
Maybe a line-reader already exists in the frontend (?) .. or maybe it should.
--
Justin
