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
gcc 4.6 warnings #55160
Comments
To analyze bpo-9880, I installed gcc-4.6. It looks like this new gcc version emits new warnings. Here is a report of py3k warnings generated with "gcc-4.6 -O3 -Wall -Wextra -Wstrict-prototypes -Wno-missing-field-initializers -Wno-unused-parameter" on AMD64. I grouped manually the warnings. sign-compare: Parser/node.c: In function 'PyNode_AddChild': unused-but-set-variable: Parser/parsetok.c: In function 'parsetok': empty-body: Parser/pgen.c: In function 'compile_rule': uninitialized: Objects/setobject.c: In function 'test_c_api': (other): ./Python/sysmodule.c: In function 'svnversion_init': |
I took a look at an example of each type. Sign-compare, Parser/node.c, 94 I presume PY_SIZE_MAX and sizeof(node) are both unsigned. The comparison is 'OK' in context in that it is preceded by and guarded by If I had written this, I might have thought about replacing all the comparisons with one check of n1->n_nchildren against some reasonable limit calculated from PY_SIZE_MAX / sizeof(node), and making both capacities unsigned. --- --- #ifdef Py_DEBUG
#define REQN(i, count) \
if (i < count) { \
fprintf(stderr, REQNFMT, count); \
Py_FatalError("REQN"); \
} else
#else
#define REQN(i, count) /* empty */
#endif Since all invocations of REQN look like " REQN(i, 1);", I presume the 'else' is there to swallow up the ';' which is added to make macro calls look like statements with a function call. I guess this is a style issue. As I remember, the suggestion to simply add '{}' to the macro would make syntax errors. --- Uninitialized, Objects/setobject.c, 2445, in test_c_api:
while (_PySet_NextEntry((PyObject *)dup, &i, &x, &hash)) {
s = _PyUnicode_AsString(x); I presume x is set by _PySet... . (If i and hash are also, they are not used). So 'may be' but not actually. --- |
gcc 4.6 bug has been fixed (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47271). So setup.py can compile extensions using gcc 4.6, and here are new warnings: |
Just a comment on those warnings about unused assignments; think about commenting them out instead of flat-out deleting them. When I ran clang over Python 2.7 I got some blow-back from deleting some assignments as some found them to be like documentation. In instances where the assignment helps explain what is going on it is best to just comment the code out. |
I can see how a comment like /* spam = eggs + ham */ could be useful, but and isolated /* foo = 0 */ where 'foo' appears nowhere else in the file seems more like confusing noise. |
You're right, Terry. It's a judgment call as to what should be simply deleted compared to commented out. |
this fixes the pickle warnings, and cleans up some (I'm pretty sure) dead code in there. the pickle tests all pass. |
This fixes every compiler warning so that Python build with -Werror on Ubuntu Oneiric alpha (gcc 4.6.1-7ubuntu1).
I don't get all the errors mentioned by haypo. At least some seem already fixed. |
My patch above fixes all the messages so that you get a clean build with the current makefile. -Wuninitialized and 'offset outside constant string' would be worth fixing but I can't reproduce them in Python. I'm personally not so keen on fixing all the signed/unsigned comparisons unless they're checked by the default build, because in my experience that has a pretty low payoff and some chance of introducing errors. |
I've fixed two more warnings, see my patch. (gcc 4.6.2) |
New changeset d7862e855274 by Victor Stinner in branch '3.2': New changeset 49b85dba251d by Victor Stinner in branch 'default': |
New changeset 2f10c1ad4b21 by Ross Lagerwall in branch 'default': New changeset 1dd43e939c07 by Ross Lagerwall in branch 'default': |
(As usual), I'm quite skeptical about this "bulk bug report"; it violates the "one bug at a time" principle, where "one bug" can roughly be defined as "cannot be split into smaller independent issues". For the cases at hand, I think it would be best if somebody with gcc 4.6 available just fixed the "easy" ones, i.e. where the code clearly improves when silenciing the warning. In these cases, I wouldn't mind if they get checked in without code review; I know some favor review for all changes, in which case a separate issue should be opened for a patch fixing a bunch of these. The more difficult ones may deserve their own issues (e.g. when it is debatable whether gcc is right to warn about the code) |
Hi, Martin, On 20 August 2012 05:25, Martin v. Löwis <report@bugs.python.org> wrote:
I heartily agree with you in general: as well as being non-atomic,
I've fixed what I believe to be all the safe/easy warnings in my patch |
fix_2warnings.diff: I don't like statement like (void)err; to kill a compiler warning. I prefer GCC __attribute__((unused)) using a macro instead. About the change on unicode_fromascii, the code changed a lot since the patch was written. The patch is outdated and not really interesting. Please open a new issue with a new patch if there are remaining warnings. I'm closing the issue because I agree with Martin von Loewis, it's better to open new issues for each warning (warning class?). |
What about backporting this fixes to 2.7? |
I don't mind them being backported, in the spirit of supporting newer tools well in 2.7. OTOH, I don't see it as necessary, either. 2.7 will eventually be phased out, and then it doesn't matter how much warnings it produces. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: