hnrklssn marked an inline comment as done.
hnrklssn added inline comments.
================
Comment at: llvm/utils/UpdateTestChecks/common.py:1286
+ if value == default_value:
+ continue
if action.dest == 'filters':
----------------
nikic wrote:
> nikic wrote:
> > nikic wrote:
> > > hnrklssn wrote:
> > > > nikic wrote:
> > > > > We should also not print the `all` argument for `--check-globals`
> > > > > argument for `version < 3`, otherwise that will introduce a spurious
> > > > > change in all such tests.
> > > > I don't know how to dynamically change the `--check-globals` option
> > > > between `store_true` and `choices`, since the behaviour can itself be
> > > > affected by which argument is passed to the `--version` option. So the
> > > > way I see it there's no way of knowing how to parse between two
> > > > different option types ahead of time. For the default when no option is
> > > > specified the parsing is the same however, so we can simply infer later
> > > > what option is implied based on the version.
> > > It's not necessary to change the option, just how it is printed. I.e. add
> > > something like this:
> > > ```
> > > if args.version < 3 and value == "all":
> > > # Don't include argument value in older versions.
> > > autogenerated_note_args += "--check-globals "
> > > continue
> > > ```
> > Ah, I get what you're saying now -- old UTC_ARGS are not accepted at all
> > anymore, and trying to update existing tests will just fail with an error!
> >
> > We do need old UTC_ARGS to work. I think you can make the argument value
> > optional using these options:
> > ```
> > nargs="?",
> > const="default",
> > default="default",
> > choices=["none", "smart", "all", "default"],
> > ```
> >
> > Or possibly omit const/default and handle None instead.
> Correction, I think the right options are:
> ```
> nargs="?",
> const="all",
> default="default",
> choices=["none", "smart", "all"],
> ```
> That is, use `"default"` is it's not specified at all, use `"all"` if only
> `--check-globals` is passed and also accept `--check-globals none|smart|all`.
Ah nice, I did not expect the library to support this use case!
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D148216/new/
https://reviews.llvm.org/D148216
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
