From: Aaron B. <bo...@gm...> - 2020-04-15 14:47:04
|
On Wed, Apr 15, 2020 at 8:47 AM Daniel Aarno <dan...@gm...> wrote: > I'm aware of these, I think VS also emits these warnings. I've considered > fixing these multiple times over the years (they are all OK btw) but in > some places changing to unsigned may actually lead to *more* bugs due to > potential wrap-around issues (i.e., where the code expects that results can > be negative) - so some care needs to be taken. In general use of unsigned > ints should be discouraged when the only purpose is to signal that a value > is never less than zero and I believe most folks these days agree that it > was a mistake to use unsigned ints in STL containers. > > That said, in some places the fix is actually trivial and should probably > be done (it's just that nobody except me has cared about it so far) - so if > you want to send some patches to fix these, I'm happy to review. > > For example, this is just ridiculous: > > inline void Arg::trimFlag(std::string& flag, std::string& value) const > > { > > int stop = 0; > > for ( int i = 0; static_cast<unsigned int>(i) < flag.length(); i++ ) > > if ( flag[i] == Arg::delimiter() ) > > ... > > Yes, exactly. I have a patch here: https://github.com/boxerab/tclap/tree/warnings By the way, I would highly recommend moving the project to Github or Gitlab. Workflow is so much nicer, and you don't get bombarded by SF ads. I was very hesitiant to register with SF discussion list. It would probably only take a few hours to sign up and migrate the repo. Not sure about issue migration. Just my 2 cents :) > On Mon, Apr 13, 2020 at 3:23 AM Aaron Boxer <bo...@gm...> wrote: > >> Hello! >> >> I am using tclap in an image compression project, where sign conversion >> can lead to subtle bugs, so I turn on "-Wall -Wconversion". I also build >> with clang, which seems to catch more warnings than gcc. >> >> I see a number of conversion warnings in tclap, for example: >> >> >> ////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// >> /home/aaron/src/grok/src/include/tclap/Arg.h:563:18: warning: implicit >> conversion changes signedness: 'int' to 'std::__cxx11::basic_string<char, >> std::char_traits<char>, std::allocator<char> >::size_type' (aka 'unsigned >> long') [-Wsign-conversion] >> if (flag[i] == Arg::delimiter()) { >> ~~~~ ^ >> /home/aaron/src/grok/src/include/tclap/Arg.h:569:34: warning: implicit >> conversion changes signedness: 'int' to 'std::__cxx11::basic_string<char, >> std::char_traits<char>, std::allocator<char> >::size_type' (aka 'unsigned >> long') [-Wsign-conversion] >> value = flag.substr(stop + 1); >> ~~~~~~ ~~~~~^~~ >> /home/aaron/src/grok/src/include/tclap/Arg.h:570:31: warning: implicit >> conversion changes signedness: 'int' to 'std::__cxx11::basic_string<char, >> std::char_traits<char>, std::allocator<char> >::size_type' (aka 'unsigned >> long') [-Wsign-conversion] >> flag = flag.substr(0, stop); >> ~~~~~~ ^~~~ >> /home/aaron/src/grok/src/include/tclap/Arg.h:579:15: warning: implicit >> conversion changes signedness: 'int' to 'std::__cxx11::basic_string<char, >> std::char_traits<char>, std::allocator<char> >::size_type' (aka 'unsigned >> long') [-Wsign-conversion] >> if (s[i] == Arg::blankChar()) return true; >> ~ ^ >> In file included from >> /home/aaron/src/grok/src/bin/jp2/grk_compress.cpp:102: >> In file included from /home/aaron/src/grok/src/include/tclap/CmdLine.h:28: >> /home/aaron/src/grok/src/include/tclap/SwitchArg.h:204:25: warning: >> implicit conversion changes signedness: 'int' to 'std::vector::size_type' >> (aka 'unsigned long') [-Wsign-conversion] >> if (argMatches(args[*i])) { >> ~~~~ ^~ >> /home/aaron/src/grok/src/include/tclap/SwitchArg.h:206:23: warning: >> implicit conversion changes signedness: 'int' to 'std::vector::size_type' >> (aka 'unsigned long') [-Wsign-conversion] >> _setBy = args[*i]; >> ~~~~ ^~ >> /home/aaron/src/grok/src/include/tclap/SwitchArg.h:210:43: warning: >> implicit conversion changes signedness: 'int' to 'std::vector::size_type' >> (aka 'unsigned long') [-Wsign-conversion] >> } else if (combinedSwitchesMatch(args[*i])) { >> ~~~~ ^~ >> /home/aaron/src/grok/src/include/tclap/SwitchArg.h:214:40: warning: >> implicit conversion changes signedness: 'int' to 'std::vector::size_type' >> (aka 'unsigned long') [-Wsign-conversion] >> if (combinedSwitchesMatch(args[*i])) >> ~~~~ ^~ >> /home/aaron/src/grok/src/include/tclap/SwitchArg.h:222:34: warning: >> implicit conversion changes signedness: 'int' to 'std::vector::size_type' >> (aka 'unsigned long') [-Wsign-conversion] >> return lastCombined(args[*i]); >> ~~~~ ^~ >> In file included from >> /home/aaron/src/grok/src/bin/jp2/grk_compress.cpp:102: >> In file included from /home/aaron/src/grok/src/include/tclap/CmdLine.h:29: >> /home/aaron/src/grok/src/include/tclap/MultiSwitchArg.h:132:25: warning: >> implicit conversion changes signedness: 'int' to 'std::vector::size_type' >> (aka 'unsigned long') [-Wsign-conversion] >> if (argMatches(args[*i])) { >> ~~~~ ^~ >> /home/aaron/src/grok/src/include/tclap/MultiSwitchArg.h:135:23: warning: >> implicit conversion changes signedness: 'int' to 'std::vector::size_type' >> (aka 'unsigned long') [-Wsign-conversion] >> _setBy = args[*i]; >> ~~~~ ^~ >> /home/aaron/src/grok/src/include/tclap/MultiSwitchArg.h:143:43: warning: >> implicit conversion changes signedness: 'int' to 'std::vector::size_type' >> (aka 'unsigned long') [-Wsign-conversion] >> } else if (combinedSwitchesMatch(args[*i])) { >> ~~~~ ^~ >> /home/aaron/src/grok/src/include/tclap/MultiSwitchArg.h:151:43: warning: >> implicit conversion changes signedness: 'int' to 'std::vector::size_type' >> (aka 'unsigned long') [-Wsign-conversion] >> while (combinedSwitchesMatch(args[*i])) ++_value; >> ~~~~ ^~ >> In file included from >> /home/aaron/src/grok/src/bin/jp2/grk_compress.cpp:102: >> In file included from /home/aaron/src/grok/src/include/tclap/CmdLine.h:30: >> In file included from >> /home/aaron/src/grok/src/include/tclap/UnlabeledValueArg.h:28: >> /home/aaron/src/grok/src/include/tclap/ValueArg.h:291:29: warning: >> implicit conversion changes signedness: 'int' to 'std::vector::size_type' >> (aka 'unsigned long') [-Wsign-conversion] >> std::string flag = args[*i]; >> ~~~~ ^~ >> In file included from >> /home/aaron/src/grok/src/bin/jp2/grk_compress.cpp:102: >> In file included from /home/aaron/src/grok/src/include/tclap/CmdLine.h:31: >> In file included from >> /home/aaron/src/grok/src/include/tclap/UnlabeledMultiArg.h:27: >> /home/aaron/src/grok/src/include/tclap/MultiArg.h:271:29: warning: >> implicit conversion changes signedness: 'int' to 'std::vector::size_type' >> (aka 'unsigned long') [-Wsign-conversion] >> std::string flag = args[*i]; >> ~~~~ ^~ >> In file included from >> /home/aaron/src/grok/src/bin/jp2/grk_compress.cpp:102: >> In file included from /home/aaron/src/grok/src/include/tclap/CmdLine.h:38: >> /home/aaron/src/grok/src/include/tclap/StdOutput.h:441:30: warning: >> implicit conversion changes signedness: 'int' to >> 'std::__cxx11::basic_string<char, std::char_traits<char>, >> std::allocator<char> >::size_type' (aka 'unsigned long') [-Wsign-conversion] >> std::string indentString(indentSpaces, ' '); >> ~~~~~~~~~~~~ ^~~~~~~~~~~~ >> /home/aaron/src/grok/src/include/tclap/StdOutput.h:448:44: warning: >> implicit conversion changes signedness: 'int' to >> 'std::__cxx11::basic_string<char, std::char_traits<char>, >> std::allocator<char> >::size_type' (aka 'unsigned long') [-Wsign-conversion] >> os << indentString << s.substr(from) << std::endl; >> ~~~~~~ ^~~~ >> /home/aaron/src/grok/src/include/tclap/StdOutput.h:459:53: warning: >> implicit conversion changes signedness: 'int' to >> 'std::__cxx11::basic_string<char, std::char_traits<char>, >> std::allocator<char> >::size_type' (aka 'unsigned long') [-Wsign-conversion] >> tooFar = s.find_first_of(splitChars, to + 1); >> ~~~~~~~~~~~~~ ~~~^~~ >> /home/aaron/src/grok/src/include/tclap/StdOutput.h:468:15: warning: >> implicit conversion changes signedness: 'int' to >> 'std::__cxx11::basic_string<char, std::char_traits<char>, >> std::allocator<char> >::size_type' (aka 'unsigned long') [-Wsign-conversion] >> if (s[to] != ' ') { >> ~ ^~ >> /home/aaron/src/grok/src/include/tclap/StdOutput.h:473:40: warning: >> implicit conversion changes signedness: 'int' to >> 'std::__cxx11::basic_string<char, std::char_traits<char>, >> std::allocator<char> >::size_type' (aka 'unsigned long') [-Wsign-conversion] >> os << indentString << s.substr(from, to - from) << '\n'; >> ~~~~~~ ^~~~ >> /home/aaron/src/grok/src/include/tclap/StdOutput.h:473:49: warning: >> implicit conversion changes signedness: 'int' to >> 'std::__cxx11::basic_string<char, std::char_traits<char>, >> std::allocator<char> >::size_type' (aka 'unsigned long') [-Wsign-conversion] >> os << indentString << s.substr(from, to - from) << '\n'; >> ~~~~~~ ~~~^~~~~~ >> /home/aaron/src/grok/src/include/tclap/StdOutput.h:476:18: warning: >> implicit conversion changes signedness: 'int' to >> 'std::__cxx11::basic_string<char, std::char_traits<char>, >> std::allocator<char> >::size_type' (aka 'unsigned long') [-Wsign-conversion] >> for (; s[to] == ' '; to++) {} >> ~ ^~ >> /home/aaron/src/grok/src/include/tclap/StdOutput.h:481:53: warning: >> implicit conversion changes signedness: 'int' to >> 'std::__cxx11::basic_string<char, std::char_traits<char>, >> std::allocator<char> >::size_type' (aka 'unsigned long') [-Wsign-conversion] >> indentString.insert(indentString.end(), secondLineOffset, ' >> '); >> ~~~~~~ ^~~~~~~~~~~~~~~~ >> /home/aaron/src/grok/src/include/tclap/StdOutput.h:444:17: warning: >> implicit conversion loses integer precision: >> 'std::__cxx11::basic_string<char, std::char_traits<char>, >> std::allocator<char> >::size_type' (aka 'unsigned long') to 'int' >> [-Wshorten-64-to-32] >> int end = s.length(); >> ~~~ ~~^~~~~~~~ >> /home/aaron/src/grok/src/include/tclap/StdOutput.h:459:24: warning: >> implicit conversion loses integer precision: >> 'std::__cxx11::basic_string<char, std::char_traits<char>, >> std::allocator<char> >::size_type' (aka 'unsigned long') to 'int' >> [-Wshorten-64-to-32] >> tooFar = s.find_first_of(splitChars, to + 1); >> ~ ~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> In file included from >> /home/aaron/src/grok/src/bin/jp2/grk_compress.cpp:102: >> /home/aaron/src/grok/src/include/tclap/CmdLine.h:488:49: warning: >> implicit conversion changes signedness: 'int' to 'std::vector::size_type' >> (aka 'unsigned long') [-Wsign-conversion] >> if (!matched && _emptyCombined(args[i])) matched = true; >> ~~~~ ^ >> /home/aaron/src/grok/src/include/tclap/CmdLine.h:494:26: warning: >> implicit conversion changes signedness: 'int' to 'std::vector::size_type' >> (aka 'unsigned long') [-Wsign-conversion] >> args[i])); >> ~~~~ ^ >> /home/aaron/src/grok/src/include/tclap/CmdLine.h:543:15: warning: >> implicit conversion changes signedness: 'int' to >> 'std::__cxx11::basic_string<char, std::char_traits<char>, >> std::allocator<char> >::size_type' (aka 'unsigned long') [-Wsign-conversion] >> if (s[i] != Arg::blankChar()) return false; >> ~ ^ >> In file included from >> /home/aaron/src/grok/src/bin/jp2/grk_compress.cpp:102: >> In file included from /home/aaron/src/grok/src/include/tclap/CmdLine.h:30: >> In file included from >> /home/aaron/src/grok/src/include/tclap/UnlabeledValueArg.h:28: >> /home/aaron/src/grok/src/include/tclap/ValueArg.h:289:25: warning: >> implicit conversion changes signedness: 'int' to 'std::vector::size_type' >> (aka 'unsigned long') [-Wsign-conversion] >> if (_hasBlanks(args[*i])) return false; >> ~~~~ ^~ >> /home/aaron/src/grok/src/bin/jp2/grk_compress.cpp:579:22: note: in >> instantiation of member function 'TCLAP::ValueArg<unsigned >> int>::processArg' requested here >> ValueArg<uint32_t> kernelBuildOptionsArg("k", >> "KernelBuild", >> ^ >> In file included from >> /home/aaron/src/grok/src/bin/jp2/grk_compress.cpp:102: >> In file included from /home/aaron/src/grok/src/include/tclap/CmdLine.h:30: >> In file included from >> /home/aaron/src/grok/src/include/tclap/UnlabeledValueArg.h:28: >> /home/aaron/src/grok/src/include/tclap/ValueArg.h:291:29: warning: >> implicit conversion changes signedness: 'int' to 'std::vector::size_type' >> (aka 'unsigned long') [-Wsign-conversion] >> std::string flag = args[*i]; >> ~~~~ ^~ >> /home/aaron/src/grok/src/include/tclap/ValueArg.h:308:36: warning: >> implicit conversion changes signedness: 'int' to 'std::vector::size_type' >> (aka 'unsigned long') [-Wsign-conversion] >> _extractValue(args[*i]); >> ~~~~ ^~ >> /home/aaron/src/grok/src/include/tclap/ValueArg.h:289:25: warning: >> implicit conversion changes signedness: 'int' to 'std::vector::size_type' >> (aka 'unsigned long') [-Wsign-conversion] >> if (_hasBlanks(args[*i])) return false; >> ~~~~ ^~ >> /home/aaron/src/grok/src/bin/jp2/grk_compress.cpp:586:22: note: in >> instantiation of member function 'TCLAP::ValueArg<unsigned >> short>::processArg' requested here >> ValueArg<uint16_t> rsizArg("Z", "RSIZ", "RSIZ", false, 0U, >> ^ >> In file included from >> /home/aaron/src/grok/src/bin/jp2/grk_compress.cpp:102: >> In file included from /home/aaron/src/grok/src/include/tclap/CmdLine.h:30: >> In file included from >> /home/aaron/src/grok/src/include/tclap/UnlabeledValueArg.h:28: >> /home/aaron/src/grok/src/include/tclap/ValueArg.h:291:29: warning: >> implicit conversion changes signedness: 'int' to 'std::vector::size_type' >> (aka 'unsigned long') [-Wsign-conversion] >> std::string flag = args[*i]; >> ~~~~ ^~ >> /home/aaron/src/grok/src/include/tclap/ValueArg.h:308:36: warning: >> implicit conversion changes signedness: 'int' to 'std::vector::size_type' >> (aka 'unsigned long') [-Wsign-conversion] >> _extractValue(args[*i]); >> ~~~~ ^~ >> /home/aaron/src/grok/src/include/tclap/ValueArg.h:289:25: warning: >> implicit conversion changes signedness: 'int' to 'std::vector::size_type' >> (aka 'unsigned long') [-Wsign-conversion] >> if (_hasBlanks(args[*i])) return false; >> ~~~~ ^~ >> /home/aaron/src/grok/src/bin/jp2/grk_compress.cpp:595:20: note: in >> instantiation of member function >> 'TCLAP::ValueArg<std::__cxx11::basic_string<char> >::processArg' requested >> here >> ValueArg<string> IMFArg("z", "IMF", "IMF profile", false, >> "", "string", >> ^ >> In file included from >> /home/aaron/src/grok/src/bin/jp2/grk_compress.cpp:102: >> In file included from /home/aaron/src/grok/src/include/tclap/CmdLine.h:30: >> In file included from >> /home/aaron/src/grok/src/include/tclap/UnlabeledValueArg.h:28: >> /home/aaron/src/grok/src/include/tclap/ValueArg.h:291:29: warning: >> implicit conversion changes signedness: 'int' to 'std::vector::size_type' >> (aka 'unsigned long') [-Wsign-conversion] >> std::string flag = args[*i]; >> ~~~~ ^~ >> /home/aaron/src/grok/src/include/tclap/ValueArg.h:308:36: warning: >> implicit conversion changes signedness: 'int' to 'std::vector::size_type' >> (aka 'unsigned long') [-Wsign-conversion] >> _extractValue(args[*i]); >> ~~~~ ^~ >> /home/aaron/src/grok/src/include/tclap/ValueArg.h:289:25: warning: >> implicit conversion changes signedness: 'int' to 'std::vector::size_type' >> (aka 'unsigned long') [-Wsign-conversion] >> if (_hasBlanks(args[*i])) return false; >> ~~~~ ^~ >> /home/aaron/src/grok/src/bin/jp2/grk_compress.cpp:609:21: note: in >> instantiation of member function 'TCLAP::ValueArg<int>::processArg' >> requested here >> ValueArg<int32_t> deviceIdArg("G", "DeviceId", "Device >> ID", false, 0U, >> ^ >> In file included from >> /home/aaron/src/grok/src/bin/jp2/grk_compress.cpp:102: >> In file included from /home/aaron/src/grok/src/include/tclap/CmdLine.h:30: >> In file included from >> /home/aaron/src/grok/src/include/tclap/UnlabeledValueArg.h:28: >> /home/aaron/src/grok/src/include/tclap/ValueArg.h:291:29: warning: >> implicit conversion changes signedness: 'int' to 'std::vector::size_type' >> (aka 'unsigned long') [-Wsign-conversion] >> std::string flag = args[*i]; >> ~~~~ ^~ >> /home/aaron/src/grok/src/include/tclap/ValueArg.h:308:36: warning: >> implicit conversion changes signedness: 'int' to 'std::vector::size_type' >> (aka 'unsigned long') [-Wsign-conversion] >> _extractValue(args[*i]); >> ~~~~ ^~ >> /home/aaron/src/grok/src/include/tclap/ValueArg.h:289:25: warning: >> implicit conversion changes signedness: 'int' to 'std::vector::size_type' >> (aka 'unsigned long') [-Wsign-conversion] >> if (_hasBlanks(args[*i])) return false; >> ~~~~ ^~ >> /home/aaron/src/grok/src/bin/jp2/grk_compress.cpp:627:21: note: in >> instantiation of member function 'TCLAP::ValueArg<unsigned >> char>::processArg' requested here >> ValueArg<uint8_t> tpArg("u", "TP", "Tile part >> generation", false, 0U, >> ^ >> In file included from >> /home/aaron/src/grok/src/bin/jp2/grk_compress.cpp:102: >> In file included from /home/aaron/src/grok/src/include/tclap/CmdLine.h:30: >> In file included from >> /home/aaron/src/grok/src/include/tclap/UnlabeledValueArg.h:28: >> /home/aaron/src/grok/src/include/tclap/ValueArg.h:291:29: warning: >> implicit conversion changes signedness: 'int' to 'std::vector::size_type' >> (aka 'unsigned long') [-Wsign-conversion] >> std::string flag = args[*i]; >> ~~~~ ^~ >> /home/aaron/src/grok/src/include/tclap/ValueArg.h:308:36: warning: >> implicit conversion changes signedness: 'int' to 'std::vector::size_type' >> (aka 'unsigned long') [-Wsign-conversion] >> _extractValue(args[*i]); >> >> >> ////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// >> >> I think this could solved by using size_t instead of int in a number of >> places. >> >> Interested to know your thoughts, >> Aaron >> >> >> >> >> _______________________________________________ >> Tclap-discuss mailing list >> Tcl...@li... >> https://lists.sourceforge.net/lists/listinfo/tclap-discuss >> > |