Date: Thu, 15 Feb 2024 23:33:08 +0000

Hi Jan, thank you very much for the feedback, it was very appreciated.

Comments below.

> Firstly, I strongly oppose returning std::tuple and I suspect the committee will see it the same way. The style of returnings pairs or tuples has been phased out, and I don't think it's going to make a comeback with this paper. It degrades code quality too much not to have meaningful names and makes the potential for misuse too great.

I have been divided between tuples and structs, I think for the most part they tuples were clear, except for "mul_wide", yeah I agree, you would have to read closely to know which one it is.

In my mind I had idealized algorithms working like this:

bool carry = false;

std::tie(out[0], carry) = std::add_carry(var1[0], var2[0], carry);

std::tie(out[1], carry) = std::add_carry(var1[1], var2[1], carry);

std::tie(out[2], carry) = std::add_carry(var1[2], var2[2], carry);

std::tie(out[3], carry) = std::add_carry(var1[3], var2[3], carry);

instead of:

std::add_carry_result result;

result = std::add_carry(var1[0], var2[0], false);

out[0] = result.sum;

result = std::add_carry(var1[1], var2[1], result .carry);

out[1] = result.sum;

result = std::add_carry(var1[2], var2[2], result .carry);

out[2] = result.sum;

But the point is that I would hope the compiler would auto-magically get rid of the intermediate structures and do everything in registers (regardless of return type).

I'm ok to make that change if others share the same opinion, even if the result is not as clean, at the end of the day the existence of these functions are far more important that my own personal preference on it.

> Furthermore, [[nodiscard]] does not belong in the wording. It's debatable whether the standard should mandate [[nodiscard]] on functions at all.

I can remove.

> The wording also uses CTAD in the return type (std::tuple with no template arguments) which is probably a mistake.

Thanks for pointing that out. It is a mistake, I forgot to escape the < > and then the browser just clobbered it when opening the html. Oops!

>> Constraints: R and T are signed or unsigned integer types ([basic.fundamental]).

>This can be simplified to just saying they are integer types.

I did not add those, that language is already in the standard. The rest just follows it to make sure that they all look the same.

> You can also improve the wording of is_div_defined. You could just use a single expression involving the conditional operator, which is better than mixing prose and multiple boolean expressions. However, I would simply phrase it as:

> Returns: true if dividend / divisor is defined.

This one was a bit tricky, right or wrong, someone could theoretically decide to define (std::numeric_limits<T>::min() / -1) as std::numeric_limits<T>::min(), and this case shouldn't be allowed for what the function tries to do.

I can rewrite it as: true if divisor != 0 and dividend / divisor is representable as a value of type T

The remainder of the suggestions are good, I will take those into consideration on the next revision, I've already started redrafting it, there will be an update soon.

Comments below.

> Firstly, I strongly oppose returning std::tuple and I suspect the committee will see it the same way. The style of returnings pairs or tuples has been phased out, and I don't think it's going to make a comeback with this paper. It degrades code quality too much not to have meaningful names and makes the potential for misuse too great.

I have been divided between tuples and structs, I think for the most part they tuples were clear, except for "mul_wide", yeah I agree, you would have to read closely to know which one it is.

In my mind I had idealized algorithms working like this:

bool carry = false;

std::tie(out[0], carry) = std::add_carry(var1[0], var2[0], carry);

std::tie(out[1], carry) = std::add_carry(var1[1], var2[1], carry);

std::tie(out[2], carry) = std::add_carry(var1[2], var2[2], carry);

std::tie(out[3], carry) = std::add_carry(var1[3], var2[3], carry);

instead of:

std::add_carry_result result;

result = std::add_carry(var1[0], var2[0], false);

out[0] = result.sum;

result = std::add_carry(var1[1], var2[1], result .carry);

out[1] = result.sum;

result = std::add_carry(var1[2], var2[2], result .carry);

out[2] = result.sum;

But the point is that I would hope the compiler would auto-magically get rid of the intermediate structures and do everything in registers (regardless of return type).

I'm ok to make that change if others share the same opinion, even if the result is not as clean, at the end of the day the existence of these functions are far more important that my own personal preference on it.

> Furthermore, [[nodiscard]] does not belong in the wording. It's debatable whether the standard should mandate [[nodiscard]] on functions at all.

I can remove.

> The wording also uses CTAD in the return type (std::tuple with no template arguments) which is probably a mistake.

Thanks for pointing that out. It is a mistake, I forgot to escape the < > and then the browser just clobbered it when opening the html. Oops!

>> Constraints: R and T are signed or unsigned integer types ([basic.fundamental]).

>This can be simplified to just saying they are integer types.

I did not add those, that language is already in the standard. The rest just follows it to make sure that they all look the same.

> You can also improve the wording of is_div_defined. You could just use a single expression involving the conditional operator, which is better than mixing prose and multiple boolean expressions. However, I would simply phrase it as:

> Returns: true if dividend / divisor is defined.

This one was a bit tricky, right or wrong, someone could theoretically decide to define (std::numeric_limits<T>::min() / -1) as std::numeric_limits<T>::min(), and this case shouldn't be allowed for what the function tries to do.

I can rewrite it as: true if divisor != 0 and dividend / divisor is representable as a value of type T

The remainder of the suggestions are good, I will take those into consideration on the next revision, I've already started redrafting it, there will be an update soon.

Received on 2024-02-15 23:33:11