bpo-42431: Fix outdated bytes comments #23458
Conversation
Also move definitions of internal macros F_LJUST etc to private header.
Include/cpython/bytesobject.h
Outdated
| @@ -2,6 +2,9 @@ | |||
| # error "this header file must not be included directly" | |||
| #endif | |||
|
|
|||
| /* Caching the hash (ob_shash) saves recalculation of a bytes' hash value. | |||
| This significantly speeds up dict lookups. */ | |||
There was a problem hiding this comment.
Maybe move this comment before ob_shash definition, in the structure.
There was a problem hiding this comment.
Or just remove it as trivial? We do not have similar note for other cached hashes, and initially this comment was about strings.
| #define Py_INTERNAL_FORMAT_H | ||
| #ifdef __cplusplus | ||
| extern "C" { | ||
| #endif |
There was a problem hiding this comment.
I'm fine with adding pycore_format.h.
But maybe we could rename pycore_bytes_methods.h to pycore_bytes.h or pycore_bytesobject.h and put everything in a single header file. It's up to you.
It seems like unicode doesn't use these constants, so maybe be more explicit in the file name and rename it to "pycore_bytesformat.h" or "pycore_bytes_format.h"?
There was a problem hiding this comment.
These constants are used both by bytes and unicode formatters. This is the reason why they are declared in the header file.
pycore_bytes_methods.h contains declarations for bytes_methods.c which contains common code and docstrings for bytes and bytearray.
| * F_SIGN '+' | ||
| * F_BLANK ' ' | ||
| * F_ALT '#' | ||
| * F_ZERO '0' |
There was a problem hiding this comment.
Would you mind to give an example of function which uses these constants?
There was a problem hiding this comment.
They are used in a number of internal functions. Their names do not matter because they are static.
Objects/bytesobject.c
Outdated
| @@ -5,6 +5,7 @@ | |||
| #include "Python.h" | |||
| #include "pycore_abstract.h" // _PyIndex_Check() | |||
| #include "pycore_bytes_methods.h" // _Py_bytes_startswith() | |||
| #include "pycore_format.h" // F_LJUST etc | |||
There was a problem hiding this comment.
Remark: I have a personal preference to only give the name of a single symbol which is "imported" (included). I do that to check sometimes if the include is still needed. One symbol is enough (not "etc.") ;-)
Also move definitions of internal macros F_LJUST etc to private header.
Also move definitions of internal macros F_LJUST etc to private header.
https://bugs.python.org/issue42431