8000 [3.5] bpo-36478: Use C89 for loops in backported pickle patch by asottile · Pull Request #12622 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

[3.5] bpo-36478: Use C89 for loops in backported pickle patch #12622

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jul 13, 2019
Merged

[3.5] bpo-36478: Use C89 for loops in backported pickle patch #12622

merged 2 commits into from
Jul 13, 2019

Conversation

asottile
Copy link
Contributor
@asottile asottile commented Mar 29, 2019 •
edited by bedevere-bot
Loading

@asottile
Copy link
Contributor Author

CC @vstinner (original patch ef33dd6)

pganssle
pganssle approved these changes Apr 7, 2019
View reviewed changes
Copy link
Member
@pganssle pganssle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, assuming that it was a deliberate choice to not include detection of regressions of this sort as part of the CI tests, which I assume @vstinner would know.

If that was an oversight, now might be a good time to try to remedy the situation (would also make it easier to tell if this PR actually solves the bug, too).

@asottile
Copy link
Contributor Author
asottile commented Apr 7, 2019

fwiw, this patch was necessary to build for deadsnakes: deadsnakes/python3.5@fe8602d

@vstinner
Copy link
Member
vstinner commented Apr 8, 2019

assuming that it was a deliberate choice to not include detection of regressions of this sort as part of the CI tests, which I assume @vstinner would know.

I'm not sure of what you means by "regression". Using GCC 8.3.1 (on Fedora 29), I get 65 compiler warnings but no warnings on _pickle.c, with "./configure -C --with-pydebug CFLAGS=-O0 && make".

https://bugs.python.org/issue36478 is just a compiler warning. I don't know why it is seen as an error. For example, GCC uses ISO C99 by default.

@vstinner
Copy link
Member
vstinner commented Apr 8, 2019

I tried gcc -std=c89, the compiler fails early on a different file:

gcc -pthread -c -Wno-unused-result -Wsign-compare -g -Og -Wall -Wstrict-prototypes -std=c89   -Werror=declaration-after-statement   -I. -I./Include    -DPy_BUILD_CORE -o Objects/listobject.o Objects/listobject.c
Objects/bytesobject.c: In function '_PyBytes_Format':
Objects/bytesobject.c:782:17: error: C++ style comments are not allowed in ISO C90
                 // %r is only for 2/3 code; 3 only code should use %a
                 ^
Objects/bytesobject.c:782:17: error: (this will be reported only once per input file)

This comment was added 4 years ago:

commit 62e977f1b6d52a973304db9b0268aece99cb6c42
Author: Ethan Furman <ethan@stoneleaf.us>
Date:   Wed Mar 11 08:17:00 2015 -0700

    Close issue23467: add %r compatibility to bytes and bytearray

I'm not sure which kind of problem we are trying to solve here.

I know that on Windows, "error: 'for' loop initial declarations are only allowed in C99 mode" might be an error, but at this point, nobody tried to build the latest Python 3.5 on Windows and the compilation goes fine on Linux with default GCC flags. There issue looks to be specific to compiler flags used by https://github.com/deadsnakes

@pganssle
Copy link
Member
pganssle commented Apr 8, 2019

I'm not sure of what you means by "regression".

Sorry, for some reason I misread the original issue and thought that you filed it, which is why I figured it was something that was detected by a buildbot.

According to PEP 7, for Python 3.5, the code should be C89 and it seems like it's one step above a pure-style refactoring if deadsnakes requires it to build.

@asottile
Copy link
Contributor Author
asottile commented Apr 8, 2019

The compiler targetted here is pretty old (but then again, so is python3.5): gcc 4.8.x

From the man page: https://gcc.gnu.org/onlinedocs/gcc-4.8.5/gcc/C-Dialect-Options.html#C-Dialect-Options

‘gnu89’
GNU dialect of ISO C90 (including some C99 features). This is the default for C code.

In gnu89:

root@5b98b67e14ee:/# gcc --version
gcc (Ubuntu 4.8.4-2ubuntu1~14.04.4) 4.8.4
Copyright (C) 2013 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

root@5b98b67e14ee:/# gcc t.c
t.c: In function 'main':
t.c:4:5: error: 'for' loop initial declarations are only allowed in C99 mode
     for (int i = 0; i < 5; i += 1) {
     ^
t.c:4:5: note: use option -std=c99 or -std=gnu99 to compile your code
root@5b98b67e14ee:/# cat t.c 
#include <stdio.h>

int main(void) {
    for (int i = 0; i < 5; i += 1) {
        printf("%d\n", i);
    }
}

@thehesiod
Copy link
Contributor

needed for debian:stretch builds with default gcc

vstinner
vstinner reviewed Apr 11, 2019
View reviewed changes
@asottile
Fix compatibility with ISO C89 needed by "gnu89" standard of GCC 4.8:…
7df8f06 
… use C89 for loops in backported pickle patch
@asottile asottile force-pushed the C89_for_loops_pickle_bpo-36478 branch from 7a93296 to 7df8f06 Compare April 11, 2019 16:20
vstinner
vstinner approved these changes Apr 11, 2019
View reviewed changes
Copy link
Member
@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

I tested manually: compilation fails with gcc -std=gnu89 without the change, but the compilation completes with the change.

@vstinner
Copy link
Member

@larryhastings: Would you mind to merge this PR to compilation support of old GCC versions?

@asottile
Copy link
Contributor Author

@larryhastings bump on this!

@larryhastings larryhastings merged commit 43a0ae9 into python:3.5 Jul 13, 2019
@bedevere-bot
Copy link

@larryhastings: Please replace # with GH- in the commit message next time. Thanks!

@larryhastings
Copy link
Contributor

Thanks for the 3.5 love!

@asottile asottile deleted the C89_for_loops_pickle_bpo-36478 branch July 13, 2019 22:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vstinner vstinner vstinner approved these changes

@pganssle pganssle pganssle approved these changes

Assignees

@larryhastings larryhastings

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@asottile @vstinner @pganssle @thehesiod @bedevere-bot @larryhastings @the-knights-who-say-ni
0