Skip to content

Re-add value_type detection to distinguish string types - #3604

Merged
nlohmann merged 2 commits into
nlohmann:developfrom
falbrechtskirchinger:stringtype-detection
Jul 28, 2022
Merged

nlohmann merged 2 commits into
nlohmann:developfrom
falbrechtskirchinger:stringtype-detection

Conversation

@falbrechtskirchinger

Copy link
Copy Markdown
Contributor

Re-add the detection of a value_type member to the is_constructible_string_type type trait.

Fixes #3602.

@falbrechtskirchinger
falbrechtskirchinger marked this pull request as draft July 23, 2022 20:30
@coveralls

coveralls commented Jul 23, 2022 •

Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 100.0% when pulling 307b828 on falbrechtskirchinger:stringtype-detection into 2d48a4d on nlohmann:develop.

@falbrechtskirchinger falbrechtskirchinger changed the title Re-add value_type detection to is_constructible_string_type trait Re-add value_type detection to distinguish string types Jul 24, 2022
@falbrechtskirchinger
falbrechtskirchinger marked this pull request as ready for review July 24, 2022 05:47
@falbrechtskirchinger
falbrechtskirchinger marked this pull request as draft July 27, 2022 05:33
@falbrechtskirchinger

falbrechtskirchinger commented Jul 27, 2022 •

Copy link
Copy Markdown
Contributor Author

Not too happy with this band-aid; will try a different approach.

Edit: Nevermind. value_type is required by the library. I'll keep working on a better type trait for string types, but don't want to rush it.
@gregmarr raised concerns that this might be a breaking change. string_t::value_type is used in a few places and string types without a value_type member wouldn't have compiled in 3.10.4 and 3.10.5 either. To be 100% sure, I'd have to grep the release headers for string_t::value_type.

@falbrechtskirchinger
falbrechtskirchinger marked this pull request as ready for review July 27, 2022 06:52

@nlohmann nlohmann left a comment

Copy link
Copy Markdown
Owner

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 self-assigned this Jul 28, 2022
@nlohmann nlohmann added this to the Release 3.11.0 milestone Jul 28, 2022
@nlohmann
nlohmann merged commit a87c188 into nlohmann:develop Jul 28, 2022
@falbrechtskirchinger
falbrechtskirchinger deleted the stringtype-detection branch July 28, 2022 20:07
nlohmann added a commit that referenced this pull request Oct 4, 2026
* Add regression test for converting json to std::variant<json> (#5066)

With 3.10.5, get<std::variant<json>>() was well-formed through the string
from_json overload, so the implicit conversion operator was a candidate
when converting json to std::variant<json>, and MSVC picked it over the
variant's converting constructor. The tightened constraints from #3427 and
#3604 (3.11.0) removed that path; this test guards against regressions.

Signed-off-by: Niels Lohmann <mail@nlohmann.me>

* Fix clang-tidy and clang 6 in the #5066 regression test

ci_clang_tidy asked for emplace_back instead of push_back. The
push_back is the point of the test: #5066 is about the implicit
conversion from json to the vector's value type, which emplace_back
would bypass. Silence the check on that line.

clang 5 and 6 cannot instantiate std::variant<json> from libstdc++ 10's
<variant> ("cannot cast private base class"), which broke
ci_test_compilers_clang (6). Tested with the CI images: clang 7 to 11
compile and pass, including clang 11 with libstdc++ 10. Skip the runtime
check for clang before 7; the static_assert still runs.

Signed-off-by: Niels Lohmann <mail@nlohmann.me>

---------

Signed-off-by: Niels Lohmann <mail@nlohmann.me>
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.

xwidgets doesn't compile with version >3.10.3

3 participants

⌂ 换网址