Skip to content

Conversation

harrishancock
Copy link
Collaborator

I belatedly recalled that I shouldn't be using functional-style casts, since they implicitly perform a const_cast. In this case, it's not a big risk, but in theory someone could change a strong bool type to some other type with a non-const operator bool(), causing a safety issue.

The type already supports static_cast, of course, but no one's going to want to use that.

I belatedly recalled that I shouldn't be using functional-style casts, since they implicitly perform a const_cast. In this case, it's not a big risk, but in theory someone could change a strong bool type to some other type with a non-const `operator bool()`, causing a safety issue.

The type already supports static_cast, of course, but no one's going to want to use that.
@harrishancock harrishancock requested review from jasnell and tewaro July 14, 2025 12:45
@harrishancock harrishancock requested review from a team as code owners July 14, 2025 12:45
Copy link

codspeed-hq bot commented Jul 14, 2025

CodSpeed Performance Report

Merging #4522 will improve performances by 18.9%

Comparing harris/2025-07-14-strong-bool-to-bool (5a484c5) with main (487fd1e)

Summary

⚡ 1 improvements
✅ 10 untouched benchmarks

Benchmarks breakdown

Benchmark BASE HEAD Change
SlowAPIWithLock[FastMethodFixture] 34.7 ms 29.2 ms +18.9%

@jasnell
Copy link
Collaborator

jasnell commented Jul 14, 2025

@anonrig ... unrelated to this PR, but I've noticed the SlowAPIWithLock benchmark appears to have fairly massive swings from one run to the next in some of these PRs. This PR, for instance, shouldn't really have impacted the benchmark at all but it's reporting an 18.9% improvement here. Likely something worth looking into at some point as I'm not sure how reliable that benchmark actually is given the variance.

@jasnell jasnell merged commit 7d7aad7 into main Jul 14, 2025
22 checks passed
@jasnell jasnell deleted the harris/2025-07-14-strong-bool-to-bool branch July 14, 2025 17:08
@anonrig
Copy link
Member

anonrig commented Jul 14, 2025

@anonrig ... unrelated to this PR, but I've noticed the SlowAPIWithLock benchmark appears to have fairly massive swings from one run to the next in some of these PRs. This PR, for instance, shouldn't really have impacted the benchmark at all but it's reporting an 18.9% improvement here. Likely something worth looking into at some point as I'm not sure how reliable that benchmark actually is given the variance.

@jasnell The problem should been fixed with #4525 which landed on main but this PR was not up to date with main. I think the iteration count is not reliable, and the PR should do some real work to avoid variance.

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