Skip to content

Conversation

@KeithBrown39423
Copy link

@KeithBrown39423 KeithBrown39423 commented Apr 26, 2025

What does this PR do?

Removes a call to bun.toFD that (I assume used to exist and was replaced) no longer works with bun.FileDescriptor.fromNative

Also replaced the custom bun.clamp function with std.math.clamp as std.math.clamp now supports floats.

How did you verify your code works?

bun run build now works when before it was throwing an error.

Remove custom clamp function written before std.math supported floats and does nothing different other than separate the two.
Replaced bun.toFD call with FileDescriptor.fromNative
@RiskyMH
Copy link
Contributor

RiskyMH commented Apr 27, 2025

I'm not sure if its related to this PR, but it looks like there are a few tests that fail on every distro except windows now:

https://buildkite.com/bun/bun/builds/15704#annotation-test/js/third_party/body-parser/express-memory-leak.test.ts

pub extern "c" fn powf(x: f32, y: f32) f32;
pub extern "c" fn pow(x: f64, y: f64) f64;

/// Restrict a value to a certain interval unless it is a float and NaN.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With a NaN input, bun.clamp was returning NaN but std.math.clamp returns the upper bound. Not sure how much that matters at the callsites. We can see if CI passes, at least, and I can investigate a little more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I didn't notice CI already finished. The CSS test failures seem related, for example: https://buildkite.com/bun/bun/builds/15704#01967459-a513-4b88-bf65-0c0b2c61805b/2080-2137

Expected: "\n/* a.css */\nh1 {\n    color: rgb(0, calc(NaN), 0);\n}\n"
Received: "/* a.css */\nh1 {\n  color: #0f0;\n}\n"

Maybe let's keep this function, but only implement it for floats, and document better why we need it instead of std.math.clamp.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also move it to css_parser.zig, maybe.

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.

3 participants