Port conversions to c++ #200

juntuu · 2021-03-07T00:59:06Z

The main funtion doing the conversion work is convert<From, To>(). The external interface is unchanged (except B -> bool).

Simple cases use just simple copy, with range check when From may exceed Tos value range.
Another overload takes additional trasformation function, if the function returns std::optional<T>, a std::nullopt signals failure and is checked
More complex cases use template specialisations

Also added template parameter to specify return type of pointer_to_values() (e9e7f41).
This changes the semantics of set_value_at() to depend on the type of the value:

T value;
set_value_at(arr, idx, value)

// was previously equivalent to
int64_t *values = (int64_t*)AV(arr);
values[idx] = value;

// is now equivalent to
T *values = (T*)AV(arr);
values[idx] = value;

This allows use of set_value_at() with types wider than, or not convertible to, int64_t.
However, this requires more care with the value parameter, it must be the correct type (correct bit width), to get the correct stride.

codereport · 2021-03-09T04:51:11Z

This is awesome! Will do a live code review on the next live stream!

codereport

This is 🔥 Thanks for opening the PR. Comments below or for the first 200 lines.

codereport · 2021-03-16T23:03:17Z

jsrc/conversions.cpp

+[[nodiscard]] constexpr auto
+in_range(V value) -> bool {
+    if constexpr (std::is_same_v<bool, V>) {
+        (void)value;


What is this for?

This seems indeed unnecessary, I think I had some compiler errors with bool at the time, but don't remember exactly what

Found the reason for the special case, however, I think I'll just disable the warning

../jsrc/conversions.cpp:29:45: error: comparison of constant ‘-9223372036854775808’ with boolean expression is always true [-Werror=bool-compare] --

jsrc/conversions.cpp

codereport · 2021-03-16T23:15:28Z

jsrc/conversions.cpp

+
+template <>
+[[nodiscard]] auto
+convert<D, bool>(J jt, array w, void *yv, D fuzz) -> bool {


Let's try to avoid the D macro

Suggested change

convert<D, bool>(J jt, array w, void *yv, D fuzz) -> bool {

convert<D, bool>(J jt, array w, void *yv, double fuzz) -> bool {

Maybe same goes for all the others

Did D -> double and I -> int64_t (and one last B -> bool)

most others are j specific structs

ps. typedefs not macros

The different width integer types (1, 2, 3, 4 bytes; UC, US...) could possibly be replaced, but seeing the compiler, I'd postpone that to whenever those typedefs are refactored.

codereport · 2021-03-16T23:17:29Z

jsrc/conversions.cpp

+template <>
+[[nodiscard]] auto
+convert<Z, D>(J jt, array w, void *yv, D fuzz) -> bool {
+    if (fuzz != 0.0) {


Is this ok?

Original test was if (fuzz), not sure if these have differences

codereport · 2021-03-16T23:22:36Z

jsrc/conversions.cpp

+    return convert<B, X>(jt,
+                         w,
+                         yv,
+                         [=](auto v) {
+                             int64_t u[] = {v};
+                             return jtvec(jt, INT, 1L, u);
+                         }) &&
+           !jt->jerr;


See if you can do something like this: (not {v} might not work.

Suggested change

return convert<B, X>(jt,

w,

yv,

[=](auto v) {

int64_t u[] = {v};

return jtvec(jt, INT, 1L, u);

}) &&

!jt->jerr;

auto name_me = [=](auto v) { return jtvec(jt, INT, 1L, {v}); };

return convert<B, X>(jt, w, yv, name_me) && !jt->jerr;

{v} won't work in the parameter list, the function takes void*

jsrc/conversions.cpp

codereport · 2021-03-16T23:30:50Z

jsrc/conversions.cpp

+    switch (mode) {
+        case XMFLR: p = e; break;
+        case XMCEIL: p = ceil(p); break;
+        case XMEXACT:
+            ASSERT(TEQ(p, e), EVDOMAIN);
+            p = e;
+            break;
+        case XMEXMT:
+            if (!TEQ(p, e)) { return jtvec(jt, INT, 0L, &iotavec[-IOTAVECBEGIN]); }
+    }


Suggested change

switch (mode) {

case XMFLR: p = e; break;

case XMCEIL: p = ceil(p); break;

case XMEXACT:

ASSERT(TEQ(p, e), EVDOMAIN);

p = e;

break;

case XMEXMT:

if (!TEQ(p, e)) { return jtvec(jt, INT, 0L, &iotavec[-IOTAVECBEGIN]); }

}

if (!TEQ(p, e)) return jtvec(jt, INT, 0L, &iotavec[-IOTAVECBEGIN]);

p = mode == XMEXACT ? e : ceil(p);

This refactor would invalidate the meaning of the modes (if the comments are up to date)

jsource/jsrc/vx.h

Lines 20 to 23 in 5f03cf2

#define XMFLR 0 /* floor, round down */

#define XMCEIL 1 /* ceiling, round up */

#define XMEXACT 2 /* exact, error if impossible */

#define XMEXMT 3 /* exact, empty if impossible */

jsrc/conversions.cpp

Also use std::copy_n() instead of std::copy() in convert() Also also, I kind of understand, but wtf: std::numeric_limits<T>::min() and std::numeric_limits<T>::lowest() Now fixed behaviour with double (and bool)

juntuu · 2021-03-17T01:35:52Z

CI compiler is giving somewhat ambiguous error:

../jsrc/conversions.cpp:565:71: error: call of overloaded ‘jtccvt(JST*&, <unnamed enum>, AD*&, AD**)’ is ambiguous
--

No idea how it thinks jtccvt() from all the functions would be overloaded...

Turned out the CI compiler had different idea of I compared to int64_t

The macro definition had parentheses, which made it work

juntuu force-pushed the port-conversions branch 2 times, most recently from b7ef90d to df695e3 Compare March 7, 2021 01:30

juntuu mentioned this pull request Mar 7, 2021

Update CI compiler version #201

Open

juntuu force-pushed the port-conversions branch from c3b8f1d to 2addff8 Compare March 7, 2021 12:01

juntuu marked this pull request as ready for review March 7, 2021 14:27

juntuu requested a review from codereport as a code owner March 7, 2021 14:27

juntuu added the c to c++ Porting C code to C++ label Mar 7, 2021

github-actions bot assigned juntuu Mar 8, 2021

juntuu force-pushed the port-conversions branch 3 times, most recently from 70472ad to d9ae65c Compare March 8, 2021 08:42

juntuu mentioned this pull request Mar 8, 2021

Remove xnum conversions #203

Draft

juntuu force-pushed the port-conversions branch from d9ae65c to 4462728 Compare March 8, 2021 09:30

juntuu force-pushed the port-conversions branch from 4462728 to 1ecd97d Compare March 9, 2021 20:47

codereport mentioned this pull request Mar 15, 2021

LiveStream #12 ToDo List #211

Closed

4 tasks

codereport requested changes Mar 16, 2021

View reviewed changes

github-actions bot requested a review from codereport March 17, 2021 00:51

juntuu force-pushed the port-conversions branch 4 times, most recently from e35c7cc to ddbcc44 Compare March 17, 2021 01:27

juntuu added 8 commits March 17, 2021 03:32

Conversions to c++

88a97f3

Add templated functions for simple conversions

2ef2302

Remove tautologically true #ifdef

223413f

Add range check to convert()

7558baf

Refactor jtDfromX()

d3187f7

Const refactor jtDfromQ()

f8d612b

Refactor jtXfromI()

d308fbf

Extract couple helper functions

bd25196

juntuu added 17 commits March 17, 2021 03:32

Remove more decl-init split

77c0105

Add helper function

bda0f83

Use helper functions more

34a427d

Parametrise pointer_to_values() return type

ab99794

Replace *AV() macros with pointer_to_values()

fe73293

Use array instead of A

300efec

Until c++17

b0069a6

Use true/false for booleans, replace C-style casts

5935520

Add stdbool.h and extern "C" guards in j.h

b528de2

Chain conversion pairs with &&

079d171

Inline pointer_to_values<void>() with correct types

acc08ba

Remove pointer_to_values() type parameter default

3b340f7

Add checked transform variant of convert()

a0379e4

Use convert() instead of raw loops

d860c28

Replace std::copy_n() with convert()

10504fc

Also use std::copy_n() instead of std::copy() in convert() Also also, I kind of understand, but wtf: std::numeric_limits<T>::min() and std::numeric_limits<T>::lowest() Now fixed behaviour with double (and bool)

Refactor conversions to complex (Z)

194cbc4

Make the CI compiler happy

4c39a04

Add missing parentheses

7229958

The macro definition had parentheses, which made it work

juntuu force-pushed the port-conversions branch 3 times, most recently from 984cba4 to 1224af8 Compare March 17, 2021 08:51

Replace I and D with correct types

c993b44

juntuu force-pushed the port-conversions branch from 1224af8 to c993b44 Compare March 17, 2021 08:58

juntuu added 4 commits March 17, 2021 11:00

Extract transformation lambda to variable

b71a020

Remove unnecessary special case

05d91c6

Use std::negate{}

2110d69

Use std::div()

86ae356

juntuu force-pushed the port-conversions branch from b4051a4 to 86ae356 Compare March 17, 2021 09:10

Disable bool-compare warning

d5cc1b9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port conversions to c++ #200

Port conversions to c++ #200

juntuu commented Mar 7, 2021 •

edited

Loading

codereport commented Mar 9, 2021

codereport left a comment

codereport Mar 16, 2021

juntuu Mar 16, 2021

juntuu Mar 17, 2021

codereport Mar 16, 2021

codereport Mar 16, 2021

juntuu Mar 17, 2021

juntuu Mar 17, 2021

codereport Mar 16, 2021

juntuu Mar 17, 2021

codereport Mar 16, 2021

juntuu Mar 17, 2021

codereport Mar 16, 2021

juntuu Mar 17, 2021

juntuu commented Mar 17, 2021 •

edited

Loading

	convert<D, bool>(J jt, array w, void *yv, D fuzz) -> bool {
	convert<D, bool>(J jt, array w, void *yv, double fuzz) -> bool {

	#define XMFLR 0 /* floor, round down */
	#define XMCEIL 1 /* ceiling, round up */
	#define XMEXACT 2 /* exact, error if impossible */
	#define XMEXMT 3 /* exact, empty if impossible */

Port conversions to c++ #200

Are you sure you want to change the base?

Port conversions to c++ #200

Conversation

juntuu commented Mar 7, 2021 • edited Loading

codereport commented Mar 9, 2021

codereport 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juntuu commented Mar 17, 2021 • edited Loading

juntuu commented Mar 7, 2021 •

edited

Loading

juntuu commented Mar 17, 2021 •

edited

Loading