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

bpo-32030: Rewrite calculate_path() #4521

Merged
merged 4 commits into from Nov 23, 2017
Merged

bpo-32030: Rewrite calculate_path() #4521

merged 4 commits into from Nov 23, 2017

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Nov 23, 2017

  • calculate_path() rewritten in Modules/getpath.c and PC/getpathp.c

  • Move global variables into a new PyPathConfig structure.

  • calculate_path():

    • Split the huge calculate_path() function into subfunctions.
    • Add PyCalculatePath structure to pass data between subfunctions.
    • Document PyCalculatePath fields.
    • Move cleanup code into a new calculate_free() subfunction
    • calculate_init() now handles Py_DecodeLocale() failures properly
    • calculate_path() is now atomic: only replace PyPathConfig
      (path_config) at once on success.
  • _Py_GetPythonHomeWithConfig() now returns an error on failure

  • Add _Py_INIT_NO_MEMORY() helper: report a memory allocation failure

  • Coding style fixes (PEP 7)

https://bugs.python.org/issue32030

* calculate_path() rewritten in Modules/getpath.c and PC/getpathp.c
* Move global variables into a new PyPathConfig structure.
* calculate_path():

  * Split the huge calculate_path() function into subfunctions.
  * Add PyCalculatePath structure to pass data between subfunctions.
  * Document PyCalculatePath fields.
  * Move cleanup code into a new calculate_free() subfunction
  * calculate_init() now handles Py_DecodeLocale() failures properly
  * calculate_path() is now atomic: only replace PyPathConfig
    (path_config) at once on success.

* _Py_GetPythonHomeWithConfig() now returns an error on failure
* Add _Py_INIT_NO_MEMORY() helper: report a memory allocation failure
* Coding style fixes (PEP 7)
* Add PyCalculatePath.prog
* Remove extern Py_GetProgramName
* calculate_path() works on a temporary configuration, and only
  writes the new full configuration at once on success
Use config->xxx rather than xxx, so later it will be possible to use
wchar_t* rather than wchar_t[] in PyPathConfig.
Mark calculate_module_search_path() as private.
@vstinner vstinner merged commit 0327bde into python:master Nov 23, 2017
@vstinner vstinner deleted the calculate_path branch November 23, 2017 16:03
@berkerpeksag
Copy link
Member

Can we stop doing unrelated PEP 7 and PEP 8 "fixes" please? It basically makes the diff unreadable. The following style has been working fine (we never introduced a bug like Apple's "goto fail") for the past 20 years:

if (foo)
    bar();

and I don't see a reason to replace all instances of that with

if (foo) {
    bar();
}

just to make them conform PEP 7.

Also, diffs like

-    if (_Py_wstat(filename, &buf) != 0)
+    if (_Py_wstat(filename, &buf) != 0) {
         return 0;
-    if (!S_ISREG(buf.st_mode))
+    }
+    if (!S_ISREG(buf.st_mode)) {
         return 0;
+    }

and

-#include <mach-o/dyld.h>
+#  include <mach-o/dyld.h>

basically make git blame useless.

I think we've updated PEP 7 to make curly braces required for new code.

@vstinner
Copy link
Member Author

Hi @berkerpeksag, I'm trying to restrict myself to not abuse the PEP 7. This PR is a giant refactoring patch on the very old calculate_path() code. I took this as an opportunity to "upgrade" the code to the latest coding style (so PEP 7 for C, in this case).

I think we've updated PEP 7 to make curly braces required for new code.

I understand that the PEP 7 asks to not push changes which only change the coding style. But here I modified almost all functions, so I chose to apply the PEP 7 style everywhere.

basically make git blame useless.

I'm using "git blame" very often, and I almost always hit multiple "refactoring" changes before finding what I was looking for. In fact, it's quite easy to navigate between versions of the code to go deeper and deeper.

and I don't see a reason to replace all instances of that with (...) to make them conform PEP 7.

For me, it's hard to modify code which has different coding styles. I don't know which coding style I'm supposed to use. It's also common that code is copied/pasted with the old coding style, spreading the bad coding style. For newcomers, it's also hard to guess what is the "correct" style.

That's why I'm slowly converting old code, line by line, to latest coding style.

@berkerpeksag
Copy link
Member

I understand that the PEP 7 asks to not push changes which only change the coding style. But here I modified almost all functions, so I chose to apply the PEP 7 style everywhere.

PEP 7 says only apply it when you change that specific part of the code. For example, it's perfectly OK to modify the following diff

#ifdef SPAM
#define EGGS 42
#endif

- if (foo)
+ if (foo && bar)
    return baz();

if (another_cond)
    return NULL;

to

#ifdef SPAM
#define EGGS 42
#endif

- if (foo)
+ if (foo && bar) {
    return baz();
+ }

if (another_cond)
    return NULL;

But it doesn't say you can do

#ifdef SPAM
- #define EGGS 42
+ #  define EGGS 42
#endif

- if (foo)
+ if (foo && bar) {
    return baz();
+ }

- if (another_cond)
+ if (another_cond) {
    return NULL;
+ }

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

4 participants