Reduce amount of warnings with pedantic compiler #159
Conversation
| @@ -457,7 +457,7 @@ class Argument { | |||
| return start; | |||
| } else if (mNumArgs <= std::distance(start, end)) { | |||
| if (auto expected = maybe_nargs()) { | |||
| end = std::next(start, *expected); | |||
| end = std::next(start, static_cast<long>(*expected)); | |||
There was a problem hiding this comment.
*expected is a std::size_t and is clearly defined as such. I'm surprised the compiler is warning about this.
There was a problem hiding this comment.
https://en.cppreference.com/w/cpp/iterator/next
because second argument for std::next is typename std::iterator_traits<InputIt>::difference_type. where InputIt difference_type is long int. In that case InputIt is it variable on this line:
https://github.com/p-ranav/argparse/blob/master/include/argparse/argparse.hpp#L1074
There was a problem hiding this comment.
I see the need, now. But, MSVC++ has 32-bit long int. The safer cast appears to be to decltype(start)::difference_type.
| @@ -620,7 +620,7 @@ class Argument { | |||
| * sign: one of | |||
| * '+' '-' | |||
| */ | |||
| static bool is_decimal_literal(std::string_view s) { | |||
There was a problem hiding this comment.
Do all these variable names need to change? Would renaming just the consume_digits parameter clear shadowing issues?
There was a problem hiding this comment.
No, but it makes code more readable. Of cause if you don't like my change on variables name, then I will revert my changes on them
There was a problem hiding this comment.
I wanted to be clear whether these naming changes affect the reported issue with pedantic warnings. I don't see a problem with the new names.
| @@ -1018,7 +1018,7 @@ class ArgumentParser { | |||
| stream << "Positional arguments:\n"; | |||
|
|
|||
| for (const auto &mPositionalArgument : parser.mPositionalArguments) { | |||
| stream.width(tLongestArgumentLength); | |||
| stream.width(static_cast<std::streamsize>(tLongestArgumentLength)); | |||
There was a problem hiding this comment.
I believe we can change the output type for ::get_length_of_longest_argument (which is private) to std::streamsize. Then these casts are no longer needed?
There was a problem hiding this comment.
Yes, it will be no longer needed. I think would be better to just change variable tLongestArgumentLength on line 1007 to std::streamsize
There was a problem hiding this comment.
e.g:
const auto& tLongestArgumentLength = static_cast<std::streamsize>(parser.get_length_of_longest_argument());There was a problem hiding this comment.
Why do you think it is better to static_cast instead of using the expected final type from the beginning? I'm not understanding the advantage you see.
| return s.substr(it - std::begin(s)); | ||
| auto consume_digits = [=](std::string_view needle) { | ||
| auto it = std::find_if_not(std::begin(needle), std::end(needle), is_digit); | ||
| return needle.substr(static_cast<std::size_t>(it - std::begin(needle))); |
There was a problem hiding this comment.
I don't see a way around this cast, but is this the right type? For containers, I thought we should use size_type, e.g. static_cast<decltype(needle)::size_type>(it - std::begin(needle)).
Yes, these all should resolve to the same type (probably std::size_t), but this may save us if the type for needle changes in the future.
There was a problem hiding this comment.
Yes for sure, it will save in future
Added -Wshadow and -Wconversion to CXX_FLAGS and fixed warnings (related to #159)
-Wshadow
-Wconversion
-Wsign-conversion