put charconv into contrib #1837

marehr · 2020-05-22T16:23:11Z

This PR

does the first task of putting the llvm version of charconv into the contrib folder.
See Current LLVM copy of to_chars is wrong for integers with more than 9 digits #1595 (comment)
splits the charconv test into two tests; one for integral types which would test the llvm implementation and one for floating types which would test our own implementation

I didn't upgrade the llvm version yet, because they had a license change to apache2 WITH llvm-exception

See

My second reason was to test the upgrade-path if we ever need to add new version of this.
And my third reason: Like always, a small change can lead to a couple of unintended CI breakage; I wanted to see if everything works with the new structure which uses the old code.

PLEASE DON'T SQUASH-MERGE THIS PR

codecov · 2020-05-22T17:01:55Z

Codecov Report

Merging #1837 into master will increase coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1837      +/-   ##
==========================================
+ Coverage   97.63%   97.67%   +0.03%     
==========================================
  Files         249      253       +4     
  Lines        9481     9551      +70     
==========================================
+ Hits         9257     9329      +72     
+ Misses        224      222       -2

Impacted Files	Coverage Δ
include/seqan3/search/search.hpp	`83.87% <0.00%> (-4.77%)`	⬇️
include/seqan3/core/simd/view_to_simd.hpp	`91.30% <0.00%> (-1.62%)`	⬇️
.../search/detail/unidirectional_search_algorithm.hpp	`98.57% <0.00%> (-1.43%)`	⬇️
...de/seqan3/argument_parser/detail/version_check.hpp	`92.56% <0.00%> (-0.18%)`	⬇️
include/seqan3/search/search_result.hpp	`100.00% <0.00%> (ø)`
include/seqan3/range/views/minimiser.hpp	`100.00% <0.00%> (ø)`
include/seqan3/core/algorithm/configuration.hpp	`100.00% <0.00%> (ø)`
include/seqan3/range/container/dynamic_bitset.hpp	`100.00% <0.00%> (ø)`
...clude/seqan3/alignment/pairwise/align_pairwise.hpp	`100.00% <0.00%> (ø)`
...lude/seqan3/alignment/pairwise/alignment_range.hpp	`100.00% <0.00%> (ø)`
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4fcbeb8...d3a8e82. Read the comment docs.

marehr · 2020-05-25T11:31:09Z

(Documentation is currently failing, but you don't see it in CI, because of #1838)

marehr · 2020-05-27T21:45:11Z

@simonsasse This can be reviewed now, documentation error was fixed

simonsasse

Looks good, thanks again for the explanation ! :)
The "body" of namespaces does not get indented right ?

marehr · 2020-05-29T11:54:27Z

Looks good, thanks again for the explanation ! :)
The "body" of namespaces does not get indented right ?

yes, you are right :)

smehringer

Mostly questions but requesting changes because of the test

include/seqan3/contrib/charconv/charconv.cpp

smehringer · 2020-06-02T05:39:46Z

include/seqan3/contrib/charconv/charconv_pre.hpp

+
+#include <seqan3/core/platform.hpp>
+
+#define SEQAN3_CONTRIB_CHARCONV_LIBCPP_BEGIN_NAMESPACE_STD namespace seqan3::contrib::charconv {


It's a little confusing that the macro name states "begin std namespace" but you are opening seqan3::contrib::charconv?

true, but I want to keep the files as close to their original as possible.

smehringer · 2020-06-02T05:43:18Z

include/seqan3/contrib/charconv/charconv_pre.hpp

+
+#define SEQAN3_CONTRIB_CHARCONV_LIBCPP_BEGIN_NAMESPACE_STD namespace seqan3::contrib::charconv {
+#define SEQAN3_CONTRIB_CHARCONV_LIBCPP_END_NAMESPACE_STD }
+// #define SEQAN3_CONTRIB_CHARCONV_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER 1 // needs to be set if compiling with MSVC


Why don't you wrap this in

#if defined (_MSC_VER) // ... #endif

? (not sure if this macro is the right one)

We don't support MSVC right now, so I think I don't need to do this. Fun fact: MSVC has full charconv support, so this header should not be included.

smehringer · 2020-06-02T05:44:53Z

include/seqan3/contrib/charconv/charconv_pre.hpp

+#define SEQAN3_CONTRIB_CHARCONV_LIBCPP_STD_VER 17
+// #define SEQAN3_CONTRIB_CHARCONV_LIBCPP_PUSH_MACROS
+// #define SEQAN3_CONTRIB_CHARCONV_LIBCPP_POP_MACROS
+// #define SEQAN3_CONTRIB_CHARCONV_LIBCPP_AVAILABILITY_TO_CHARS
+#define SEQAN3_CONTRIB_CHARCONV_LIBCPP_FUNC_VIS
+#define SEQAN3_CONTRIB_CHARCONV_LIBCPP_ENUM_VIS
+#define SEQAN3_CONTRIB_CHARCONV_LIBCPP_TYPE_VIS
+// #define SEQAN3_CONTRIB_CHARCONV_LIBCPP_CONSTEXPR constexpr
+#define SEQAN3_CONTRIB_CHARCONV_LIBCPP_HIDDEN
+#define SEQAN3_CONTRIB_CHARCONV_LIBCPP_INLINE_VISIBILITY
+#define SEQAN3_CONTRIB_CHARCONV_LIBCPP_ASSERT(condition, message) assert(condition)


Can you shortly explain the neccessity of these macros? (I am not doubting them, just not understanding them)

These are all prefixed MACROS that the original implementation https://github.com/llvm/llvm-project/blob/d27489645bb8829a952f79e3b211202844238391/libcxx/include/charconv uses.

Our previous approach of renaming and seqan3-ifing external things was wrong, because an update is REALLY hard to do. The new approach is to keep the original sources as close as they are and only do changes that are deemed necessary.

smehringer · 2020-06-02T05:46:03Z

include/seqan3/contrib/charconv/charconv_pre.hpp

+using std::is_integral;
+using std::is_unsigned;
+using std::is_signed;
+// using std::make_unsigned;


can you drop a comment why this is out commented?
Same for using std::enable_if;

I can remove them :)

I removed them :)

smehringer · 2020-06-02T05:52:29Z

test/unit/std/charconv_int_test.cpp

+
+    EXPECT_EQ(res.ptr, buffer.data() + buffer.size());
+    EXPECT_EQ(res.ec, std::errc::value_too_large);
+}


Can you add the test mentioned in the issue #1595 that checks whether the error still occurs?
Sorry if it is there but I overlooked it.

No, like the description of this PR said

I didn't upgrade the llvm version yet, because they had a license change to apache2 WITH llvm-exception

and

My second reason was to test the upgrade-path if we ever need to add new version of this.
And my third reason: Like always, a small change can lead to a couple of unintended CI breakage; I wanted to see if everything works with the new structure which uses the old code.

This means that I just copied the original file of charconv that we already used.
The next PR will update the charconv implementation and adds a test case.

The reason for this:

Use a known working version of charconv so that the infra structure change is known to work.

The new code has a new license which I wanted to discuss with you.

Test the update-path. That means is a upgrade of charconv as painless as possible.?

Code directly taken from * https://github.com/llvm/llvm-project/blob/d27489645bb8829a952f79e3b211202844238391/libcxx/LICENSE.TXT * https://github.com/llvm/llvm-project/blob/d27489645bb8829a952f79e3b211202844238391/libcxx/include/charconv * https://github.com/llvm/llvm-project/blob/d27489645bb8829a952f79e3b211202844238391/libcxx/src/charconv.cpp

…avoid clashes with clang

Some remarks: The float overloads need to be defined in the seqan3::contrib::charconv namespace. We can't add the overload directly in the `std` namespace, because the float overload would then overshadow the integral definition of to_chars and from_chars.

smehringer

See gitter message

smehringer · 2020-06-11T11:33:14Z

Marcel wanted to clean the commit history before merging

marehr requested review from a team and simonsasse and removed request for a team May 22, 2020 16:23

marehr force-pushed the charconv-use-contrib branch 3 times, most recently from a838ef9 to e0d4219 Compare May 27, 2020 17:11

simonsasse approved these changes May 28, 2020

View reviewed changes

marehr requested review from a team and smehringer and removed request for a team May 29, 2020 11:54

smehringer requested changes Jun 2, 2020

View reviewed changes

marehr added 8 commits June 6, 2020 10:13

[MISC] charconv: prefix _LIBCPP macros with SEQAN3_CONTRIB_LIBCPP to …

3ee0334

…avoid clashes with clang

[MISC] charconv: inline functions in charconv.cpp

06f9f31

[MISC] charconv: comment out includes

d00316a

[MISC] charconv: define macros s.t. any c++ compiler can compile it

9fb93e1

[DOC] charconv: cond out internals

f2be816

[TEST] charconv: split charconv test case

3a2cd28

marehr force-pushed the charconv-use-contrib branch from e0d4219 to d3a8e82 Compare June 6, 2020 08:16

marehr requested a review from smehringer June 6, 2020 08:17

smehringer reviewed Jun 10, 2020

View reviewed changes

smehringer approved these changes Jun 10, 2020

View reviewed changes

marehr merged commit ffcf239 into seqan:master Jun 12, 2020

marehr deleted the charconv-use-contrib branch June 12, 2020 19:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

put charconv into contrib #1837

put charconv into contrib #1837

marehr commented May 22, 2020 •

edited

Loading

codecov bot commented May 22, 2020 •

edited

Loading

marehr commented May 25, 2020

marehr commented May 27, 2020

simonsasse left a comment

marehr commented May 29, 2020 •

edited

Loading

smehringer left a comment

smehringer Jun 2, 2020

marehr Jun 2, 2020

smehringer Jun 2, 2020

marehr Jun 2, 2020

smehringer Jun 2, 2020

marehr Jun 2, 2020

smehringer Jun 2, 2020

marehr Jun 2, 2020

marehr Jun 7, 2020

smehringer Jun 2, 2020

marehr Jun 2, 2020 •

edited

Loading

smehringer left a comment

smehringer commented Jun 11, 2020


		#include <seqan3/core/platform.hpp>

		#define SEQAN3_CONTRIB_CHARCONV_LIBCPP_BEGIN_NAMESPACE_STD namespace seqan3::contrib::charconv {

put charconv into contrib #1837

put charconv into contrib #1837

Conversation

marehr commented May 22, 2020 • edited Loading

PLEASE DON'T SQUASH-MERGE THIS PR

codecov bot commented May 22, 2020 • edited Loading

Codecov Report

marehr commented May 25, 2020

marehr commented May 27, 2020

simonsasse left a comment

Choose a reason for hiding this comment

marehr commented May 29, 2020 • edited Loading

smehringer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marehr Jun 2, 2020 • edited Loading

Choose a reason for hiding this comment

smehringer left a comment

Choose a reason for hiding this comment

smehringer commented Jun 11, 2020

marehr commented May 22, 2020 •

edited

Loading

codecov bot commented May 22, 2020 •

edited

Loading

marehr commented May 29, 2020 •

edited

Loading

marehr Jun 2, 2020 •

edited

Loading