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

Tighten Int.to bounds and add twos-complement bitcount #1273

Merged
merged 1 commit into from Jul 22, 2018

Conversation

tiehuis
Copy link
Member

@tiehuis tiehuis commented Jul 21, 2018

Tightens the to function bounds and adds a twos-complement bit count variant which is useful for converting from comptime_int to a fixed size in the self-hosted compiler.

I've renamed bitcount as well to be clearer about what it is actually doing. I haven't explored the zero-sized integer case issues at all yet.

@andrewrk
Copy link
Member

LGTM.

zig/src-self-hosted/ir.zig

Lines 1735 to 1736 in 58c5f94

break :fits (from_int.positive or from_int.eqZero() or int.key.is_signed) and
int.key.bit_count >= from_int.bitcount();

So I would change this to:

break :fits (from_int.positive or from_int.eqZero() or int.key.is_signed) and 
int.key.bit_count >= from_int.bitCountTwosComp(); 

Does that look right?

@tiehuis
Copy link
Member Author

tiehuis commented Jul 22, 2018

Think you'd want something more like this:

if (!int.key.is_signed and !from_int.positive) {
    break :fits false;
}

const req_bits = from_int.bitCountTwosComp() + @boolToInt(from_int.positive and int.key.is_signed);
break :fits int.key.bit_count >= req_bits

Signed never fits in an unsigned target and we need one extra bit for a positive source to a signed target.

@tiehuis
Copy link
Member Author

tiehuis commented Jul 22, 2018

With the special-cased zero fitting in any target:

if (from_int.eqZero()) {
    break :fits true;
}

@tiehuis tiehuis merged commit 07b6a3d into master Jul 22, 2018
@tiehuis tiehuis deleted the bigint-twos-comp branch July 22, 2018 05:48
@tiehuis
Copy link
Member Author

tiehuis commented Jul 22, 2018

It may make sense to add a fits function to Int itself for this purpose since this seems pretty general. It'd pretty much be like to except we would bother returning the converted value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants