-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
[MNT]: Where to use _api.check_in_list
#24828
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
Comments
It is true that passing a tuple is a bit faster, but the overhead of the function call is enormous compared to everything else anyways so micro-optimizing lists vs. tuples doesn't seem to make much sense.
check1 is basically check_in_list, check2 is a simpler version that instead checks a single value (which covers virtually all calls to check_in_list). Note that this only times the successful case, as that should take precedence over the failing case. |
A new insight: For short methods that are often called with the correct argument and have some conditional execution anyway, it is better to use case 3 as error message generation rather than checking early. |
The advantage of case 1 and 2 is that the list passed to With case 3 we could add additional values to the if-else control flow but forget to update the On a more general note: I think the timing discussion is meaningless micro-optimiztation. We're talking about 100-300ns per We don't have tens of thousands of them. Unless you can show you gain a measurable speedup for real applictations, the performance is not an argument for the preferred code structure. OTOH as said before readability and error-safety are better in case 1 and 2. |
Summary
There's been some discussion how the "best" way to write if-clauses depending on a text parameter in combination with
_api.check_in_list
, e.g. #24486 (comment)Proposed fix
There are three primary ways in the code base:
_api.check_in_list
for generating an error message once we know that no option is matching.Personally, I'm in favor of 2. Primarily as it skips a redundant check and that it is not possible to get 100% coverage in case 1. Also, it is often the case that the
_api.check_in_list
is done in another place in the code (like in the constructor), so option 3 may lead to a delayed error.I know that @timhoffm prefers 1 as that is more likely to lead to errors if a case is missed in the if-part.
(In addition, this should really be called
_api.check_in_tuple
and tuples being used as this will have some performance benefits when it comes to compiled code. Parts of the code use tuples already though...)The text was updated successfully, but these errors were encountered: