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.

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

Pointless comparison warnings #2971

Merged
merged 3 commits into from
Jul 11, 2022

Conversation

federico-busato
Copy link
Contributor

@federico-busato federico-busato commented Jul 5, 2022

The fix addresses the following errors/warnings in fmt-9.0.0 with MICROSOFT MSC 191125506/GCC-7.5.0

\fmt/chrono.h(1399): warning #186-D: pointless comparison of unsigned integer with zero
          detected during:
            instantiation of "Int fmt::v9::detail::to_nonnegative_int(T, Int) [with T=unsigned long long, Int=long long, <unnamed>=0]" 
(1688): here
fmt/chrono.h(206): warning #186-D: pointless comparison of unsigned integer with zero
          detected during:
            instantiation of "To fmt::v9::safe_duration_cast::safe_duration_cast<To,FromRep,FromPeriod,<unnamed>,<unnamed>>(std::chrono::duration<FromRep, FromPeriod>, int &) [with To=std::chrono::duration<unsigned long long, std::ratio<1LL, 1LL>>, FromRep=unsigned long long, FromPeriod=std::micro, <unnamed>=0, <unnamed>=0]" 
(1436): here
fmt/core.h(408): warning #186-D: pointless comparison of unsigned integer with zero
          detected during:
            instantiation of "auto fmt::v9::detail::to_unsigned(Int)->std::make_unsigned<Int>::type [with Int=size_t]"
fmt/include/fmt/format.h(555): here

@vitaut
Copy link
Contributor

vitaut commented Jul 7, 2022

Thanks for the PR. Could you provide a godbolt repro that triggers these warnings?

@federico-busato
Copy link
Contributor Author

I realized that the warning is reproducible only with nvcc (Nvidia Compiler) independently from the host compiler (GCC, mvsc, etc.). Unfortunately, fmt is not available on godbolt with CUDA C++.
Just to show an example, considering https://github.com/fmtlib/fmt/blob/master/include/fmt/core.h#L408, this is the full log

fmt/include/fmt/core.h(408): warning #186-D: pointless comparison of unsigned integer with zero
          detected during:
            instantiation of "auto fmt::v9::detail::to_unsigned(Int)->std::make_unsigned<Int>::type [with Int=size_t]"
fmt/include/fmt/format.h(555): here
            instantiation of "auto fmt::v9::detail::fill_n(T *, Size, char)->T * [with T=char, Size=size_t]"
fmt/include/fmt/format.h(824): here
            instantiation of "fmt::v9::basic_memory_buffer<T, SIZE, Allocator>::basic_memory_buffer(const Allocator &) [with T=char, SIZE=500UL, Allocator=std::allocator<char>]"
fmt/include/fmt/printf.h(562): here
            instantiation of "auto fmt::v9::vsprintf(const S &, fmt::v9::basic_format_args<fmt::v9::basic_printf_context_t<fmt::v9::type_identity_t<Char>>>)->std::__cxx11::basic_string<Char, std::char_traits<Char>, std::allocator<Char>> [with S=fmt::v9::basic_string_view<char>, Char=char]"
fmt/include/fmt/printf.h(581): here
            instantiation of "auto fmt::v9::sprintf(const S &, const T &...)->std::__cxx11::basic_string<Char, std::char_traits<Char>, std::allocator<Char>> [with S=const char *, T=<int>, Char=char]"

and this is the code that triggers the error:

const int v = my_value;
fmt::sprintf("my_file", v);

@vitaut
Copy link
Contributor

vitaut commented Jul 9, 2022

Is this warning enabled by default in nvcc?

@federico-busato
Copy link
Contributor Author

yes, it is enabled by default (checked with nvcc CUDA 11.6)

@vitaut vitaut merged commit 0db43cf into fmtlib:master Jul 11, 2022
@vitaut
Copy link
Contributor

vitaut commented Jul 11, 2022

Merged, thanks.

@federico-busato federico-busato deleted the pointless-comparison branch July 14, 2022 19:02
mtremer referenced this pull request in ipfire/ipfire-2.x Nov 28, 2022
- Update from version 9.0.0 to 9.1.0
- Update of rootfile
- Changelog
    9.1.0 - 2022-08-27
	* ``fmt::formatted_size`` now works at compile time
		  `#3026 <https://github.com/fmtlib/fmt/pull/3026>`_
			  For example (`godbolt <https://godbolt.org/z/1MW5rMdf8>`__):
			   .. code:: c++
			     #include <fmt/compile.h>
			     int main() {
			       using namespace fmt::literals;
			       constexpr size_t n = fmt::formatted_size("{}"_cf, 42);
			       fmt::print("{}\n", n); // prints 2
			     }
	* Fixed handling of invalid UTF-8
		  `#3038 <https://github.com/fmtlib/fmt/pull/3038>`_,
		  `#3044 <https://github.com/fmtlib/fmt/pull/3044>`_,
		  `#3056 <https://github.com/fmtlib/fmt/pull/3056>`_
	* Improved Unicode support in ``ostream`` overloads of ``print``
		  `#2994 <https://github.com/fmtlib/fmt/pull/2994>`_,
		  `#3001 <https://github.com/fmtlib/fmt/pull/3001>`_,
		  `#3025 <https://github.com/fmtlib/fmt/pull/3025>`_
	* Fixed handling of the sign specifier in localized formatting on systems with
	   32-bit ``wchar_t``
		  `#3041 <https://github.com/fmtlib/fmt/issues/3041>`_).
	* Added support for wide streams to ``fmt::streamed``
		  `#2994 <https://github.com/fmtlib/fmt/pull/2994>`_
	* Added the ``n`` specifier that disables the output of delimiters when
	   formatting ranges
		  `#2981 <https://github.com/fmtlib/fmt/pull/2981>`_,
		  `#2983 <https://github.com/fmtlib/fmt/pull/2983>`_
			  For example (`godbolt <https://godbolt.org/z/roKqGdj8c>`__):
			   .. code:: c++
			     #include <fmt/ranges.h>
			     #include <vector>
			     int main() {
			       auto v = std::vector{1, 2, 3};
			       fmt::print("{:n}\n", v); // prints 1, 2, 3
			     }
	* Worked around problematic ``std::string_view`` constructors introduced in C++23
		  `#3030 <https://github.com/fmtlib/fmt/issues/3030>`_,
		  `#3050 <https://github.com/fmtlib/fmt/issues/3050>`_
	* Improve handling (exclusion) of recursive ranges
		  `#2968 <https://github.com/fmtlib/fmt/issues/2968>`_,
		  `#2974 <https://github.com/fmtlib/fmt/pull/2974>`_
	* Improved error reporting in format string compilation
		  `#3055 <https://github.com/fmtlib/fmt/issues/3055>`_
	* Improved the implementation of
		  `Dragonbox <https://github.com/jk-jeon/dragonbox>`_, the algorithm used for
		   the default floating-point formatting
		  `#2984 <https://github.com/fmtlib/fmt/pull/2984>`_
	* Fixed issues with floating-point formatting on exotic platforms.
	* Improved the implementation of chrono formatting
		  `#3010 <https://github.com/fmtlib/fmt/pull/3010>`_
	* Improved documentation
		  `#2966 <https://github.com/fmtlib/fmt/pull/2966>`_,
		  `#3009 <https://github.com/fmtlib/fmt/pull/3009>`_,
		  `#3020 <https://github.com/fmtlib/fmt/issues/3020>`_,
		  `#3037 <https://github.com/fmtlib/fmt/pull/3037>`_
	* Improved build configuration
		  `#2991 <https://github.com/fmtlib/fmt/pull/2991>`_,
		  `#2995 <https://github.com/fmtlib/fmt/pull/2995>`_,
		  `#3004 <https://github.com/fmtlib/fmt/issues/3004>`_,
		  `#3007 <https://github.com/fmtlib/fmt/pull/3007>`_,
		  `#3040 <https://github.com/fmtlib/fmt/pull/3040>`_
	* Fixed various warnings and compilation issues
		  `#2969 <https://github.com/fmtlib/fmt/issues/2969>`_,
		  `#2971 <https://github.com/fmtlib/fmt/pull/2971>`_,
		  `#2975 <https://github.com/fmtlib/fmt/issues/2975>`_,
		  `#2982 <https://github.com/fmtlib/fmt/pull/2982>`_,
		  `#2985 <https://github.com/fmtlib/fmt/pull/2985>`_,
		  `#2988 <https://github.com/fmtlib/fmt/issues/2988>`_,
		  `#3000 <https://github.com/fmtlib/fmt/issues/3000>`_,
		  `#3006 <https://github.com/fmtlib/fmt/issues/3006>`_,
		  `#3014 <https://github.com/fmtlib/fmt/issues/3014>`_,
		  `#3015 <https://github.com/fmtlib/fmt/issues/3015>`_,
		  `#3021 <https://github.com/fmtlib/fmt/pull/3021>`_,
		  `#3023 <https://github.com/fmtlib/fmt/issues/3023>`_,
		  `#3024 <https://github.com/fmtlib/fmt/pull/3024>`_,
		  `#3029 <https://github.com/fmtlib/fmt/pull/3029>`_,
		  `#3043 <https://github.com/fmtlib/fmt/pull/3043>`_,
		  `#3052 <https://github.com/fmtlib/fmt/issues/3052>`_,
		  `#3053 <https://github.com/fmtlib/fmt/pull/3053>`_,
		  `#3054 <https://github.com/fmtlib/fmt/pull/3054>`_

Signed-off-by: Adolf Belka <adolf.belka@ipfire.org>
Reviewed-by: Michael Tremer <michael.tremer@ipfire.org>
rapids-bot bot pushed a commit to rapidsai/rmm that referenced this pull request Dec 6, 2022
This reverts commit 7bf7a76.

cc: @kkraus14 There's a handful of issues this caused downstream. We'll be sorting through these and fixing them but need to quickly revert this for now.

Currrent problems:

1. rmm's meta.yaml doesn't correctly specify spdlog as a build dep. It's [incorrectly listed as a runtime dep](https://github.com/rapidsai/rmm/blob/7bf7a763a3b6a32a7a57f900de35e2e5f30f17c0/conda/recipes/librmm/meta.yaml#L56). This means that rmm [pulls spdlog via CPM with rapids-cmake](https://github.com/rapidsai/rmm/actions/runs/3595940512/jobs/6056061203#step:6:595) `-- CPM: adding package spdlog@1.8.5 (v1.8.5)` and then clobbers its own installed spdlog headers:

```
2022-12-01T19:46:46.2167513Z ClobberWarning: This transaction has incompatible packages due to a shared path.
2022-12-01T19:46:46.2168766Z   packages: conda-forge/linux-64::spdlog-1.10.0-h924138e_0, file:///tmp/conda-bld-output/linux-64::librmm-23.02.00a-cuda11_gc328a18d_11
2022-12-01T19:46:46.2169686Z   path: 'include/spdlog/async.h'
```

2. spdlog 1.10.0 doesn't work with nvcc. We need spdlog 1.11.0 to get fmt 9.1.0, which contains [a necessary fix](fmtlib/fmt#2971). However, there's some uncertainty about how to handle the spdlog conda-forge package which [no longer vendors fmt](conda-forge/spdlog-feedstock#50) while rapids-cmake is still using the upstream repo which continues to vendor fmt. Does this inconsistency matter, as long as we pin `fmt=9.1.0` in the conda package cases? @kkraus14 It looks like you weighed in on this matter [here](conda-forge/spdlog-feedstock#53) and [here](conda-forge/conda-forge-pinning-feedstock#3654). If you have insights that can help us with this quandary, please share!

Authors:
  - Bradley Dice (https://github.com/bdice)

Approvers:
  - Mark Harris (https://github.com/harrism)
  - Ray Douglass (https://github.com/raydouglass)

URL: #1176
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants