test: Add util_ArgParsing test #17390

I really don't like setting these in stone. Let's test desired behavior. Instead of adding tests for the current insane behavior (which relies on implementation details of atoi), I think we need to work on getting errors to the user for these.

I think it is easier to review "fixes" when they are also documented with test changes.

I'm not sure about that. It could just as well bite anyone that dares to change this parsing in the future. This behavior was never documented so no one should be relying on it. (and a test is essentially "this should work" documentation)

I really don't like setting these in stone.

Like Marco is saying the goal of the test is to make it easier not harder to fix these confusing parsings in the future. Easier as a developer because you can test the effects of your change with a simple make check, easier as a reviewer to underand the change. Easier as a developer because after implementing the desired behavior you don't have to write a new test to gain coverage, but can just update a line in the existing test.

I agree with laanwj's objection broadly that it would be bad to add tests which indirectly rely on parsing behaviors, because they would be fragile and difficult to update when parsing is cleaned up. But this is a simple, direct test added specifically so we can be confident that when we intend to make parsing changes, they work as expected, and that when we intend to make other changes, we don't unknowingly break parsing.

I'm still planning on reviewing #17385 today, too. Vast majority of changes there look very good and can go a new pr even if #17385 won't be reopened.

OK, fair enough. To be clear I agree with adding this test conceptually, and some of the additional cases like "spaces around the argument should be ignored" seem decent but say, we probably don't want a test for "what atoi regards as whitespace depends on your locale" :cry: anyhow, if this is with the understanding that these aren't supposed to be guaranteed to be supported for the infinite future, I'm ok with this list

Maybe a comment that says the tests are fragile and meant to be removed could help?

Maybe a comment that says the tests are fragile and meant to be removed could help?

There is a general comment already that "Some of these cases could be ambiguous or surprising to users, and might be worth triggering errors or warning in the future", but I could also note something like "// Confusing parsing, should be changed or disallowed in the future" on specific cases if desired.

but say, we probably don't want a test for "what atoi regards as whitespace depends on your locale"

I didn't think of that but definitely agree.

"// Confusing parsing, should be changed or disallowed in the future" on specific cases if desired.

I think that's a good idea, to split the list into "normal" and "expected" cases and "parser details leaking through please don't do this" cases.

"// Confusing parsing, should be changed or disallowed in the future" on specific cases if desired.

I think that's a good idea, to split the list into "normal" and "expected" cases and "parser details leaking through please don't do this" cases.

I started to do this, but wasn't actually sure which specific cases people may want to change in the future, and figured it would be best to leave decisions to future PRs actually implementing fixes, rather than add my opinions or speculation here. For example I think leading and trailing spaces should probably just be ignored and not made into errors, and probably 5.0 and 5.5 should be errors but maybe 5. should be allowed. Unclear whether numbers with extra leading 0's should be allowed because maybe they could be mistaken for octal. Personally I think it's strange to interpret empty string "" as true and space " " as false (especially when after #15934 and #16545 we could begin to treat -flag and -flag= distinctly), and I also think we could support true false yes no for boolean flags in the future, but who knows what people are doing with existing bools today. Basically people could have opinions about every single case here except maybe the 0 case. Even the 5 case could arguably trigger an error or warning in the future if specified as the value to a boolean flag.

I also think we could support true false yes no for boolean flags in the future

Sure, but even in that case, they probably shouldn't all evaluate as 0/false. 5. and even 5.0 such at least do what could be remotely expected (truncating decimal). Accepting faux hexadecimal is pretty bad though (which is why I initially commented on this line specifically)

But I agree this might not be the place for this discussion. I'm sorry for starting this in the first place. I honestly didn't know anyone had such strong opinions about this, and would have let this sleeping dog lie if I knew.

I honestly didn't know anyone had such strong opinions about this, and would have let this sleeping dog lie if I knew.

Heh, I will talk to you all day about settings. But thanks for starting this. Previously, I assumed we actually had pretty good test coverage for parsing settings strings. I think filling the gap can prevent confusing and potentially dangerous bugs like #14523 (comment) in the future.

I also think we could support true false yes no for boolean flags in the future

Sure, but even in that case, they probably shouldn't all evaluate as 0/false.

Of course yes. By "support" I meant interpret true as true instead of false

nit, if you have to push again maybe include BOOST_CHECK(error.empty());.