Skip to content

Conversation

@pfgithub
Copy link
Collaborator

@pfgithub pfgithub commented May 6, 2025

Waiting on #20494

@robobun
Copy link
Collaborator

robobun commented May 6, 2025

Updated 7:03 PM PT - Jun 30th, 2025

@pfgithub, your commit 42596f2 has 2 failures in Build #19506:


🧪   To try this PR locally:

bunx bun-pr 19492

That installs a local version of the PR into your bun-19492 executable, so you can run:

bun-19492 --bun

@pfgithub pfgithub marked this pull request as ready for review June 17, 2025 01:26
@pfgithub pfgithub requested review from a team, 190n and nektro and removed request for a team June 17, 2025 01:26
190n
190n previously approved these changes Jun 17, 2025
pub fn getClassName(this: JSValue, global: *JSGlobalObject, ret: *ZigString) void {
pub fn getClassName(this: JSValue, global: *JSGlobalObject, ret: *ZigString) bun.JSError!void {
var scope: bun.JSC.CatchScope = undefined;
scope.init(global, @src(), .assertions_only);
Copy link
Contributor

Choose a reason for hiding this comment

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

i think this should be .enabled otherwise it will never think there is an exception. .assertions_only is for when you have a way other than the CatchScope to tell if an exception was thrown (like a return value) and you only need the CatchScope because it will call exception().

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fromJSHostCallGeneric uses .assertions_only itself so it might be wrong

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 hadn't seen that function, yeah it is wrong

    /// You have another way to detect exceptions and are only using the CatchScope to prove that
    /// exceptions are checked.
    ///
    /// This CatchScope will only do anything when assertions are enabled. Otherwise, init and
    /// deinit do nothing and it always reports there is no exception.
    assertions_only,

pfgithub and others added 2 commits June 30, 2025 20:46
@pfgithub pfgithub force-pushed the pfg/abort-signal branch from cdca597 to 2b40119 Compare July 1, 2025 01:09
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.

4 participants