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

Fix issue#1939: Cast character to unsigned for comparison #2144

Merged
merged 1 commit into from
May 28, 2020

Conversation

XyFreak
Copy link

@XyFreak XyFreak commented May 27, 2020

Addresses #1939

Hi,

I ran into #1939 today with GCC 10.1 on arm. I didn't want to cause any duplicate issues so I appologize if recycling that issue is causing any inconveniences.

The issue is reproducible with the following example with GCC 10.1 on x86 using tag 3.7.3 as well as develop:

g++ -c -Wall -Wextra -Werror -std=c++11 -funsigned-char -I json/include -x c++ -o /dev/null - << EOF
#include <nlohmann/json.hpp>
EOF
...
json/include/nlohmann/detail/input/lexer.hpp:1370:24: error: comparison is always true due to limited range of data type [-Werror=type-limits]
 1370 |             if ('\x00' <= c and c <= '\x1F')
      |                 ~~~~~~~^~~~
cc1plus: all warnings being treated as errors

GCC 9.2 does not issue a warning for this on my machine.

User @jaredgrubb already proposed this fix in #1940 and it is indeed most likely the best solution:

Looking at the assembly generated by GCC, Clang and MSVC, c gets compared with unsigned ops on x86. On platforms where char is already unsigned, c gets compared with unsigned ops as well (duh). Casting c to unsigned would change nothing code-gen wise and it is still clear what is being tested for.


Alternatively a three-way-comparison function could be implemented. This would silence the warning:

constexpr bool three_way_compare(char lower, char value, char upper) noexcept {
    return lower <= value && value <= upper;
}

(I am not really a fan of that idea but I thought I'd at least mention it.)

@XyFreak XyFreak requested a review from nlohmann as a code owner May 27, 2020 12:21
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 23051df on XyFreak:gcc10_type_limits into d70d06a on nlohmann:develop.

Copy link
Owner

@nlohmann nlohmann left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@nlohmann nlohmann linked an issue May 28, 2020 that may be closed by this pull request
@nlohmann nlohmann self-assigned this May 28, 2020
@nlohmann nlohmann added this to the Release 3.8.0 milestone May 28, 2020
@nlohmann nlohmann merged commit 61832af into nlohmann:develop May 28, 2020
@nlohmann
Copy link
Owner

Thanks!

@nlohmann
Copy link
Owner

nlohmann commented May 28, 2020


🔖 Release item

This issue/PR will be part of the next release of the library. This template helps preparing the release notes.

Type

  • ✨ New Feature
  • 🐛 Bug Fix
  • ⚡️ Improvement
  • 🔨 Further Change
  • 🔥 Deprecated function

Description


algitbot pushed a commit to alpinelinux/aports that referenced this pull request Jan 18, 2021
Due to `-Werror`, this fails to build on gcc >= 10.0.1. This has been
[reported][0] to json upstream, and fixed in this [pr][1].

Backport the fix to horizon

[0]:nlohmann/json#1939
[1]:nlohmann/json#2144
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compile warning on architectures that are not x86
3 participants