Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce amount of warnings with pedantic compiler #159

Closed
wants to merge 2 commits into from
Closed

Reduce amount of warnings with pedantic compiler #159

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0f86fdc
Reduce amount of warnings with pedantic compiler
vnepogodin Feb 4, 2022
ee1f3bc
Reduce amount of warnings with pedantic compiler
vnepogodin Feb 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 27 additions & 27 deletions include/argparse/argparse.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*expected is a std::size_t and is clearly defined as such. I'm surprised the compiler is warning about this.

Copy link
Author

@vnepogodin vnepogodin Feb 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@skrobinson skrobinson Feb 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the need, now. But, MSVC++ has 32-bit long int. The safer cast appears to be to decltype(start)::difference_type.

if (std::any_of(start, end, Argument::is_optional)) {
throw std::runtime_error("optional argument in parameter sequence");
}
Expand Down Expand Up @@ -620,7 +620,7 @@ class Argument {
* sign: one of
* '+' '-'
*/
static bool is_decimal_literal(std::string_view s) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do all these variable names need to change? Would renaming just the consume_digits parameter clear shadowing issues?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

static bool is_decimal_literal(std::string_view str) {
auto is_digit = [](auto c) constexpr {
switch (c) {
case '0':
Expand All @@ -640,15 +640,15 @@ class Argument {
};

// precondition: we have consumed or will consume at least one digit
auto consume_digits = [=](std::string_view s) {
auto it = std::find_if_not(std::begin(s), std::end(s), is_digit);
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)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes for sure, it will save in future

};

switch (lookahead(s)) {
switch (lookahead(str)) {
case '0': {
s.remove_prefix(1);
if (s.empty())
str.remove_prefix(1);
if (str.empty())
return true;
else
goto integer_part;
Expand All @@ -662,70 +662,70 @@ class Argument {
case '7':
case '8':
case '9': {
s = consume_digits(s);
if (s.empty())
str = consume_digits(str);
if (str.empty())
return true;
else
goto integer_part_consumed;
}
case '.': {
s.remove_prefix(1);
str.remove_prefix(1);
goto post_decimal_point;
}
default:
return false;
}

integer_part:
s = consume_digits(s);
str = consume_digits(str);
integer_part_consumed:
switch (lookahead(s)) {
switch (lookahead(str)) {
case '.': {
s.remove_prefix(1);
if (is_digit(lookahead(s)))
str.remove_prefix(1);
if (is_digit(lookahead(str)))
goto post_decimal_point;
else
goto exponent_part_opt;
}
case 'e':
case 'E': {
s.remove_prefix(1);
str.remove_prefix(1);
goto post_e;
}
default:
return false;
}

post_decimal_point:
if (is_digit(lookahead(s))) {
s = consume_digits(s);
if (is_digit(lookahead(str))) {
str = consume_digits(str);
goto exponent_part_opt;
} else {
return false;
}

exponent_part_opt:
switch (lookahead(s)) {
switch (lookahead(str)) {
case eof:
return true;
case 'e':
case 'E': {
s.remove_prefix(1);
str.remove_prefix(1);
goto post_e;
}
default:
return false;
}

post_e:
switch (lookahead(s)) {
switch (lookahead(str)) {
case '-':
case '+':
s.remove_prefix(1);
str.remove_prefix(1);
}
if (is_digit(lookahead(s))) {
s = consume_digits(s);
return s.empty();
if (is_digit(lookahead(str))) {
str = consume_digits(str);
return str.empty();
} else {
return false;
}
Expand Down Expand Up @@ -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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it will be no longer needed. I think would be better to just change variable tLongestArgumentLength on line 1007 to std::streamsize

Copy link
Author

@vnepogodin vnepogodin Feb 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e.g:

const auto& tLongestArgumentLength = static_cast<std::streamsize>(parser.get_length_of_longest_argument());

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

stream << mPositionalArgument;
}

Expand All @@ -1027,7 +1027,7 @@ class ArgumentParser {
<< "Optional arguments:\n";

for (const auto &mOptionalArgument : parser.mOptionalArguments) {
stream.width(tLongestArgumentLength);
stream.width(static_cast<std::streamsize>(tLongestArgumentLength));
stream << mOptionalArgument;
}

Expand Down