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

argparse: remove redundant len() #104273

Closed
buraksaler opened this issue May 7, 2023 · 3 comments
Closed

argparse: remove redundant len() #104273

buraksaler opened this issue May 7, 2023 · 3 comments

Comments

@buraksaler
Copy link
Contributor

buraksaler commented May 7, 2023

Feature or enhancement

I decreased calling of redundant len() function.

Pitch

(Explain why this feature or enhancement should be implemented and how it would be used.
Add examples, if applicable.)

Previous discussion

Linked PRs

@itamaro
Copy link
Contributor

itamaro commented May 7, 2023

hello @buraksaler!

it is not clear from this issue what len() calls you are referring to and why they are redundant.

looking at the linked PR, I'm inferring you refer to the argparse module, where you suggest replacing 3 len() calls with one.
while the change looks functionally correct to me, any change has inherent risks, so making such a change should be justifiable.
can you explain the benefits of making this change? it looks like the intention could have been improving performance by caching the result of a function call, in which case, you should provide benchmarking results (micro or macro) or profiling data showing the improvement from making this change.

@terryjreedy terryjreedy changed the title redundant len() calling argparse: remove redundant len() May 7, 2023
@terryjreedy
Copy link
Member

Justification: indent is a parameter of the helper function get_lines. It is not rebound within the function. Its length is therefore a constant within the function. Hence len(indent) is only needed once.

Calculating common subexpressions just once is a common optimization. I don't think benchmarking is needed.

@terryjreedy terryjreedy added performance Performance or resource usage and removed type-feature A feature request or enhancement labels May 7, 2023
@hauntsaninja
Copy link
Contributor

hauntsaninja commented May 7, 2023

I guess I'm okay with the specific PR, but in general we discourage microoptimisations in cold code, especially if done without any kind of benchmarking. These cost reviewer time, can hurt readability and if poorly done run the risk of regressions. I certainly wouldn't welcome PRs doing CSE wherever possible for calls to len in the standard library.

You'd need several millions of lines of help for this to add up to anything meaningful.

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

No branches or pull requests

4 participants