On Tue, Jun 09, 2020 at 11:46:24AM +0200, Pavel Stehule wrote: > po 8. 6. 2020 v 23:30 odesÃlatel Justin Pryzby <pry...@telsasoft.com> 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