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 cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits