Should I create a bug report for such simple fixes?

@zhangyangyu Thank you for reviewing! I've removed the empty lines and also realized that checking for// prefix doesn't make sense.

And it's better to return True instead of return result in UNC condition

Do you mean thatSetEnvironmentVariable return value should be ignored?

Do you mean that SetEnvironmentVariable return value should be ignored?

@izbyshev , No, I mean before when it's a UNC path,TRUE is returned. Now,result(the return value ofGetCurrentDirectoryW) is returned. Although usually there should be no difference, it's better not to do this. I have no idea about the comparison.

Although usually there should be no difference, it's better not to do this.

Actually, there mustnever be any difference, otherwise it is a bug (ifresult is0 afterGetCurrentDirectoryW, we shouldn't even check for UNC condition). I can addassert(result) before checking for UNC, but IMO changing the code to explicitly returnTRUE only in case of UNC path would unnecessarily complicate it.

but IMO changing the code to explicitly return TRUE only in case of UNC path would unnecessarily complicate it.

The function is declared to returnBOOL, so it's expected to returnTRUE orFALSE. If you want to return any integer, just declare it asint. In the extreme case, is it my fault or the API's fault ifresult == TRUE is false for a function declaredBOOL?

@zhangyangyu I totally misunderstood your point, sorry. I thought you were arguing thatresult may somehow be zero before UNC condition check.

If you worry about somebody usingBOOL constants for testing for truth, I think it's better to simply useint since the only caller usesint for its ownresult.

Don't change the signature plz. Just add another check to let it returnTRUE. It's not that bad.

@zhangyangyu Done!

@izbyshev Seems you can't remove the check for//:https://github.com/python/cpython/blob/master/Lib/pathlib.py#L127. I could find some other samples by searching that UNC starts with forward slashes.

And the code would be more explicit if you still useis_unc_path andreturn is_unc_path ? TRUE : result.

@zhangyangyu// or even/\ can be used when you pass paths to Win32 APIs, but you can't get them back fromGetCurrentDirectory because they are normalized.

If you want to go deeper, the originalis_unc_path name is also misleading because a path starting with\\ is not always a UNC path. It could also be a "local device" path (\\.\nul) or a "root local device" path (\\?\C:\). Seethis post if you're interested. So I've changed the test to be more explicit.

Regardingreturn is_unc_path ? TRUE : result, this would violate your own concern about returning anint from a function that should returnBOOL.

Okay. But I would suggest changing the checks by a new issue if you are sure and asking our Windows gurus to decide, I really can't decide on that since I don't Windows :-(. Leave this issue only about the memory issue.

Regarding return is_unc_path ? TRUE : result, this would violate your own concern about returning an int from a function that should return BOOL.

Why? ifis_unc_path is true, we always returnTRUE now, if not,result is always the return value ofSetEnvironmentVariableW, aBOOL. But wait, the Windows doc of it says it's return value is a just a nonzero but notTRUE. :-(

@zhangyangyu

Leave this issue only about the memory issue.

I wanted to take the opportunity to perform a minor cleanup by changing path checks since it doesn't distract from this small issue. But if you insist, I can revert the changes and use the original checks.

Why? if is_unc_path is true, we always return TRUE now, if not, result is always the return value of SetEnvironmentVariableW, a BOOL. But wait, the Windows doc of it says it's return value is a just a nonzero but not TRUE. :-(

If you care about explicit code, the type ofresult isint, and I don't think that making a person track data flow to verify that it's in fact can only be assigned to aBOOL value at the particular program point is explicit, especially since it involves looking at MSDN or headers to checkSetEnvironmentVariableW signature. :)

Sorry for much bikeshedding. But believe me it's not a good idea to do the cleanup in this issue. Feel free to open a new issue to push that. Of course, thanks for this contribution!

Thank you for reviewing. I don't think it's worth opening a new issue for cleanup since the checks are correct, they are just a bit less clean and optimal than they could be.

Thanks@izbyshev for the PR, and@zhangyangyu for merging it 🌮🎉.. I'm working now to backport this PR to: 2.7, 3.6, 3.7.
🐍🍒⛏🤖

GH-5945 is a backport of this pull request to the3.7 branch.

Sorry,@izbyshev and@zhangyangyu, I could not cleanly backport this to2.7 due to a conflict.
Please backport usingcherry_picker on command line.
cherry_picker 3e197c7a6740d564ad52fb7901c07d5ff49460f5 2.7

GH-5946 is a backport of this pull request to the3.6 branch.

GH-5947 is a backport of this pull request to the2.7 branch.