Correct test of OptionsImpl argc type (Was: Correct type for std::array size() result)

[wrowe]

Description:
The std::array API uses std::size_t for the size() nelts count.
This is a larger sized int than 'int', which causes this code
to break on Windows compilation.

Follow the conventional declaration of main(int argc, ...
and adjust the tests of OptionsImpl accordingly.

Original Description:

Use the stdcxx type for conformance and portability.

Signed-off-by: Sunjay Bhatia sbhatia@pivotal.io
Signed-off-by: William A Rowe Jr wrowe@pivotal.io

Risk Level: Low
Testing: Local on Windows, Linux gcc
Docs Changes: N/A
Release Notes: N/A

[bryce-anderson]

This generally seems like the right thing to do when considering array sizes etc but is this the only case where this fails to compile on Windows? There are a lot of examples of for (int i = .. in the code base.

[sunjayBhatia]

This generally seems like the right thing to do when considering array sizes etc but is this the only case where this fails to compile on Windows? There are a lot of examples of for (int i = .. in the code base.

@bryce-anderson This pattern of changes around std::size_t are not restricted to this PR, there are more but we've been trying to do them in small batches to ensure we don't have big changesets. We've also not delved into all the extension code but will do so.

As for the for (int i = .. portion that was changed, that was required here b/c of the comparison between an int and std::size_t, signed and unsigned types. If we need to continue to change those as they come up we can do so

[bryce-anderson]

While I'm not a core maintainer so my opinion is mostly for funzies, I generally approve of cleaning up size types to be more consistent with container size types so I'm 👍 on this.

@bryce-anderson This pattern of changes around std::size_t are not restricted to this PR, there are more but we've been trying to do them in small batches to ensure we don't have big changesets. We've also not delved into all the extension code but will do so.

Sounds good. I assumed so, but was curious.

As for the for (int i = .. portion that was changed, that was required here b/c of the comparison between an int and std::size_t, signed and unsigned types. If we need to continue to change those as they come up we can do so

Sorry, I wasn't clear. I understand why you made the change in this commit and was just noting that this should probably be done in most places where there is a for (int i = .., which sounds like part of your plan since they may not compile on windows. 😄

[wrowe]

I [...] was just noting that this should probably be done in most places where there is a for (int i = .., which sounds like part of your plan since they may not compile on windows. 😄

In many cases this is simply not necessary. For example, int can often be the index without ambiguity, it is the size of size_t or smaller, and depending on the comparison the signedness isn't an issue.

We are resolving this in a number of other files, but they don't need to be corrected en-mass.

[wrowe]

We are resolving this in a number of other files, but they don't need to be corrected en-mass.

And sometimes the best solution will be an auto. But not in this case, where it is the declarations of args or retval types that are at issue. The alternate patch to this file is to downcast to int, which introduces it's own set of issues.

[wrowe]

After looking at your observations, it is pretty clear the main source/ was correct. Our test/ was supposed to emulate main(argc, argv), so I think the current PR is the more correct solution.