bpo-30291 Changes to launcher so as to allow py -3-32,...#1488
bpo-30291 Changes to launcher so as to allow py -3-32,...#1488zooba merged 9 commits intopython:masterfrom
Conversation
|
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA). Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA. This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. Thanks again to your contribution and we look forward to looking at it! |
|
Note: CLA Signed but not processed yet. |
| */ | ||
| BOOL result = (p != NULL) && *p; /* Default to false if empty string or null pointer. */ | ||
|
|
||
| while (result && isdigit(*p)) /* Require a major version */{ |
There was a problem hiding this comment.
You're not requiring a major version -- just skipping leading digits. What if the version string is ".7-32"? ISTM, you should initially set BOOL result = p && isdigit(*p); . Then you can skip remaining digits.
Also, this should be calling iswdigit. The debug CRT raises an exception if isdigit is passed a value greater than 255. As things stand I think py.exe always links statically with a non-debug implementation, but it's still not a good practice to use isdigit with 16-bit wchar_t values.
There was a problem hiding this comment.
Thanks for the feedback, initial check for digit added & all isdigit calls in my changed sections replaced with iswdigit.
| if (result && (*p == L'.')) /* Allow . for major minor separator.*/ | ||
| { | ||
| result = isdigit(*++p); /* Must be at least one digit */ | ||
| while (isdigit(*++p)) ; /* Skip an more Digits */ |
There was a problem hiding this comment.
What if *++p in the previous statement on 1071 was the trailing null and result is false, i.e. the version string is something like "2."? Now you're reading past the end of the string.
There was a problem hiding this comment.
Good point - addressed.
| 3.6-64 | ||
| 3-64 | ||
| */ | ||
| BOOL result = (p != NULL) && *p &&!iswdigit(*p); /* Default to false if empty string or null pointer. */ |
There was a problem hiding this comment.
result should be true if the first character is a digit, and false for a NULL pointer or empty string, e.g. result = (p != NULL) && iswdigit(*p). You don't need to check *p separately since iswdigit is false for a nul character, but it doesn't hurt if you want to keep that in. I'm just not understanding why you would negate the value of iswdigit(*p).
There was a problem hiding this comment.
That is what I get for not re-running the tests, typo.
|
It looks like the-knights-who-say-ni are getting a little over enthusiastic @brettcannon @brettcannon brettcannon removed the CLA not signed label 6 hours ago |
|
Issue with the-knights-who-say-ni was the missing GitHub User Name in my profile. Resolved on minor change to ensure that help messages don't wrap on 80 col displays. |
| BOOL result = (p != NULL) && *p && iswdigit(*p); /* Default to false if empty string or null pointer. */ | ||
|
|
||
| while (result && iswdigit(*p)) /* Require a major version */{ | ||
| ++p; /* Skip leading digit(s) */} |
There was a problem hiding this comment.
Really don't like where the braces are here. No braces would be fine, or put them on separate lines so they are more obvious, but currently I think it's a serious comprehension issue.
|
Just the one style comment from me, but I'm going to insist it should be fixed (broken window syndrome and all that). Otherwise, it looks fine. |
|
Added response to review by @zooba + removed a number of C++ style comments in the original code (converted to C comments - also broken window syndrome). |
|
C99 comments are totally fine in master these days, so don't start fixing them everywhere (different if code is going to be backported), but it seems like you didn't actually fix the braces that I mentioned. |
|
@zooba You were quite right about the obscurity of the braces - I didn't see them when looking to fix and thought that the problem was with the mixed parenthesis style of the previous line. |
… etc.