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

gh-119192: Remove #ifndef SLOW_SUM check from sum(), use #if 1 #119193

Closed
wants to merge 2 commits into from

Conversation

sobolevn
Copy link
Member

@sobolevn sobolevn commented May 20, 2024

Or I might be wrong and it is actually useful :)
In this case, we might want to document it maybe? Or add to ./configure?

@gvanrossum
Copy link
Member

I don't think I own this file any more, and I have no idea what this is about -- maybe @tim-one does? It looks like the git blame isn't useful -- the people blamed are just the ones who merged a branch.

@tim-one
Copy link
Member

tim-one commented May 20, 2024

I already commented on the Issue Report: no strong opinion, but I'd personally change it to #if 1. I can understand why Raymond wants an easy way to turn it off (for investigating stuff), but editing 1 character is easy enough for me 😉, and there's no possibility then that other code could #define the name by mistake. Defer to Raymond, though. (It was absolutely never intended that this be user-configurable.)

@gvanrossum
Copy link
Member

Okay let's just leave it alone.

@rhettinger rhettinger removed their assignment May 20, 2024
@sobolevn sobolevn changed the title gh-119192: Remove #ifndef SLOW_SUM check from sum() gh-119192: Remove #ifndef SLOW_SUM check from sum(), use #if 1 May 20, 2024
@sobolevn
Copy link
Member Author

Thanks a lot for the feedback, I followed the idea described in #119192 (comment)

So, it is up to module owners to decide: to close this PR or to use #if 1 (which I prefer, since using undocumented names can be problematic).

Copy link
Contributor

@erlend-aasland erlend-aasland left a comment

Choose a reason for hiding this comment

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

I think Tim's suggestion of making this #if 1 instead of using a macro is fine. FWIW, I would never leave such a personal debugging tool in the code base.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd skip news for this, FWIW.

@gvanrossum
Copy link
Member

I still vote for leaving it as is. In the future, people who are curious about this will find the issue here and that will explain it to them.

@rhettinger
Copy link
Contributor

rhettinger commented May 20, 2024

FWIW, the current flag was recently useful to me while testing the improved accuracy summation feature. An #if 1 would have make testing much harder because I would have needed have needed to constantly sync two nearly identical builds to see which tests passes for each path.

Also, the alternate code paths have been present for almost two decades -- there isn't a real world problem being solved here.

BTW, is not a "personal debugging tool". Any developer who wants to do future work on this (perhaps adding a high-accuracy path for complex numbers) would benefit. There is a forseeable recurring need to be able to compare a specialized path with a base case.

@gvanrossum gvanrossum closed this May 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants