Issue4850
This issue tracker has been migrated to GitHub,
and is currently read-only.
For more information,
see the GitHub FAQs in the Python's Developer Guide.
Created on 2009-01-05 20:48 by belopolsky, last changed 2022-04-11 14:56 by admin. This issue is now closed.
Files | ||||
---|---|---|---|---|
File name | Uploaded | Description | Edit | |
issue4850.diff | belopolsky, 2009-01-05 23:06 |
Messages (16) | |||
---|---|---|---|
msg79199 - (view) | Author: Alexander Belopolsky (belopolsky) * | Date: 2009-01-05 20:48 | |
quick_int_allocs, quick_neg_int_allocs, tuple_zero_allocs, and fast_tuple_allocs are exported in -DCOUNT_ALLOCS builds. They should get a conventional _Py_ prefix. Also since tp_allocs is now Py_ssize_t, these should be redefined as Py_ssize_t as well. See msg79194 . |
|||
msg79208 - (view) | Author: Alexander Belopolsky (belopolsky) * | Date: 2009-01-05 23:06 | |
Attached patch is fairly straightforward with only one caveat: instead of prefixing unlist_types_without_objects flag with _Py_, I made it static. This may not be the right thing if it is intended to be accessible from third party modules (it is not used outside of object.c in python code). I am not sure how it is supposed to be used because it appears to be always 0. (Maybe the intent is to change it manually in specialized builds or in debugger.) I am not proposing to move declarations to header files with this patch. That change would cause a massive recompile for everyone and if a decision is made that globals' declarations belong to header files, this should probably be done it one sweep with issue4805 . |
|||
msg79231 - (view) | Author: Alexander Belopolsky (belopolsky) * | Date: 2009-01-06 05:22 | |
Martin, Can you comment on whether unlist_types_without_objects should be global? Svn blame point to you for introducing it. Thanks. $ svn blame Objects/object.c | grep unlist_types_without_objects 45527 martin.v.loewis int unlist_types_without_objects; |
|||
msg79246 - (view) | Author: Martin v. Löwis (loewis) * | Date: 2009-01-06 10:28 | |
Making it static is fine. I don't recall what kind of customization I did when I introduced it. I'm skeptical about the entire issue, though. Why is it that these variables should be prefixed? I disagree that all names need to be prefixed: those only present in certain debug builds need not. |
|||
msg79247 - (view) | Author: Marc-Andre Lemburg (lemburg) * | Date: 2009-01-06 10:30 | |
On 2009-01-06 11:28, Martin v. Löwis wrote: > Martin v. Löwis <martin@v.loewis.de> added the comment: > > Making it static is fine. I don't recall what kind of customization I > did when I introduced it. > > I'm skeptical about the entire issue, though. Why is it that these > variables should be prefixed? I disagree that all names need to be > prefixed: those only present in certain debug builds need not. For completeness and to make things easier for all developers, all exported symbols should be prefixed with _Py or Py - regardless of how their export is enabled. |
|||
msg79253 - (view) | Author: Martin v. Löwis (loewis) * | Date: 2009-01-06 12:28 | |
> For completeness I don't think completeness is a useful thing to have here. > and to make things easier for all developers, It is not easier, but more difficult. It now requires a change, whereas leaving things as-is requires no change. > all exported symbols should be prefixed with _Py or Py - regardless > of how their export is enabled. Ah - but they aren't exported symbols. |
|||
msg79254 - (view) | Author: Marc-Andre Lemburg (lemburg) * | Date: 2009-01-06 13:01 | |
On 2009-01-06 13:28, Martin v. Löwis wrote: > Martin v. Löwis <martin@v.loewis.de> added the comment: > >> For completeness > > I don't think completeness is a useful thing to have here. > >> and to make things easier for all developers, > > It is not easier, but more difficult. It now requires a change, > whereas leaving things as-is requires no change. Sure it does: consistent rules always make things easier, since they avoid doubt. >> all exported symbols should be prefixed with _Py or Py - regardless >> of how their export is enabled. > > Ah - but they aren't exported symbols. I'm only referring to exported symbols. Static globals, of course, don't need such a prefix. |
|||
msg79259 - (view) | Author: Martin v. Löwis (loewis) * | Date: 2009-01-06 13:22 | |
> I'm only referring to exported symbols. Static globals, of course, > don't need such a prefix. Ok, the symbols in question are not exported from pythonxy.dll. |
|||
msg79263 - (view) | Author: Marc-Andre Lemburg (lemburg) * | Date: 2009-01-06 13:45 | |
On 2009-01-06 14:22, Martin v. Löwis wrote: > Martin v. Löwis <martin@v.loewis.de> added the comment: > >> I'm only referring to exported symbols. Static globals, of course, >> don't need such a prefix. > > Ok, the symbols in question are not exported from pythonxy.dll. Right, because for MS VC you have to explicitly mark symbols for DLL export which is done via the PyAPI_* macros. However, they are still exported from the object files, so can cause name clashes with other libraries you link with. On Unix, libpythonX.X.a always includes those symbols and they are also in the global symbol namespace when running the executable and dynamically loading other libraries. Even production builds contain a few such symbols which require the _Py or Py prefix (or need to be made static) - these are for Python 2.6 and 2.7: * asdl_int_seq_new * asdl_seq_new Here's the grep line I used for that: nm libpython2.6.a | egrep ' [TD] ([^P_]|P[^y]|_[^P])' | sort |
|||
msg79265 - (view) | Author: Alexander Belopolsky (belopolsky) * | Date: 2009-01-06 14:57 | |
On Tue, Jan 6, 2009 at 7:28 AM, Martin v. Löwis <report@bugs.python.org> wrote: .. > It is not easier, but more difficult. It now requires a change, > whereas leaving things as-is requires no change. I actually agree with Martin on this one. I would not touch this if not for the Py_ssize_t vs. int issue. I am only +0 on renaming: if type change and format spec changes go in, renaming is cost free. |
|||
msg79267 - (view) | Author: Marc-Andre Lemburg (lemburg) * | Date: 2009-01-06 15:53 | |
On 2009-01-06 15:57, Alexander Belopolsky wrote: > Alexander Belopolsky <belopolsky@users.sourceforge.net> added the comment: > > On Tue, Jan 6, 2009 at 7:28 AM, Martin v. Löwis <report@bugs.python.org> wrote: > .. >> It is not easier, but more difficult. It now requires a change, >> whereas leaving things as-is requires no change. > > I actually agree with Martin on this one. I would not touch this if > not for the Py_ssize_t vs. int issue. I am only +0 on renaming: if > type change and format spec changes go in, renaming is cost free. I don't follow you: those symbols are not meant for public use anyway, so we can easily change them without any "costs". The same goes for any of the private, but still exported symbols in the API, ie. all _Py* APIs. |
|||
msg79271 - (view) | Author: Alexander Belopolsky (belopolsky) * | Date: 2009-01-06 16:27 | |
On Tue, Jan 6, 2009 at 10:53 AM, Marc-Andre Lemburg <report@bugs.python.org> wrote: .. > I don't follow you: those symbols are not meant for public use anyway, > so we can easily change them without any "costs". The same goes for any > of the private, but still exported symbols in the API, ie. all _Py* > APIs. This is the same that I said: "renaming is cost free" if it is done together with the type change. Martin was right that there is cost associated with any change. The benefit of "completeness" for a specialized build would not justify even having this discussion IMO. On the other hand since the type change needs to be done in order to make the code correct on a 64 bit platform and compile without warnings with gcc, a patch is needed anyways. Since we are already paying the cost of change, renaming is free. There is no disagreement between you and me here. |
|||
msg79311 - (view) | Author: Martin v. Löwis (loewis) * | Date: 2009-01-07 09:33 | |
> However, they are still exported from the object files, Ah. Those are "global symbols", not "exported symbols"; "export" is a concept specific to Win32. > so can cause > name clashes with other libraries you link with. See, and in this specific case, they can't, because they are used only in a debug build. Furthermore, they all have names that are unlikely to collide. Even if they get a _Py_ prefix, there could still be a conflict. > Even production builds contain a few such symbols which require > the _Py or Py prefix (or need to be made static) - these are for > Python 2.6 and 2.7: > > * asdl_int_seq_new > * asdl_seq_new No. They don't require the Py_ prefix, because they already have the asdl_ prefix. |
|||
msg79344 - (view) | Author: Alexander Belopolsky (belopolsky) * | Date: 2009-01-07 15:49 | |
On Wed, Jan 7, 2009 at 4:33 AM, Martin v. Löwis <report@bugs.python.org> wrote: > > Martin v. Löwis <martin@v.loewis.de> added the comment: > .. Furthermore, they all have names that are > unlikely to collide. Even if they get a _Py_ prefix, there could > still be a conflict. > My main interest in this patch are the type and %d to %zd change, but by throwing in the name change, I unintentionally made it more controversial. Would it help if I resubmit the patch without name changes or is this something that a committer can take care of if the patch is partially accepted? I already stated that I am only +0 on the name change and that "+" is mostly motivated by the fact that I already made the changes in my sandbox. Other than that, I don't think it matters. Let's stop here and refocus the type change. >> Even production builds contain a few such symbols which require >> the _Py or Py prefix (or need to be made static) - these are for >> Python 2.6 and 2.7: >> >> * asdl_int_seq_new >> * asdl_seq_new > > No. They don't require the Py_ prefix, because they already > have the asdl_ prefix. That's true, but asdl_ prefix is not a well-known prefix reserved for Python symbols. Someone who is debugging linker errors and sees something like "asdl_seq_new: symbol not found" will be hard pressed to realize that he/she forgot to link python library or linked an old version. Also a grep through nm output that Marc-Andre did is a good check to run from time to time and there is no reason to have false positives. |
|||
msg79359 - (view) | Author: Marc-Andre Lemburg (lemburg) * | Date: 2009-01-07 18:32 | |
On 2009-01-07 10:33, Martin v. Löwis wrote: > Martin v. Löwis <martin@v.loewis.de> added the comment: > >> However, they are still exported from the object files, > > Ah. Those are "global symbols", not "exported symbols"; "export" > is a concept specific to Win32. Not at all. .so share libs and .a libs on Unix provide exports just as well. >> so can cause >> name clashes with other libraries you link with. > > See, and in this specific case, they can't, because they are used > only in a debug build. Furthermore, they all have names that are > unlikely to collide. Even if they get a _Py_ prefix, there could > still be a conflict. > >> Even production builds contain a few such symbols which require >> the _Py or Py prefix (or need to be made static) - these are for >> Python 2.6 and 2.7: >> >> * asdl_int_seq_new >> * asdl_seq_new > > No. They don't require the Py_ prefix, because they already > have the asdl_ prefix. Oh please. The convention is that *all* Python exported symbols should start with either _Py or Py. That's an old convention which was introduced with the Great Renaming in Python 1.4. The above symbols violate this convention, as do any symbols that get additionally exported in debug builds or any other variant build of Python. I consider it a bug that Python still exports symbols that do not follow the convention. This makes it harder to tell whether a symbol originated in Python or some other extension or linked object/library file. |
|||
msg79361 - (view) | Author: Martin v. Löwis (loewis) * | Date: 2009-01-07 18:43 | |
> My main interest in this patch are the type and %d to %zd change, but > by throwing in the name change, I unintentionally made it more > controversial. Would it help if I resubmit the patch without name > changes or is this something that a committer can take care of if the > patch is partially accepted? I think it is indeed better to focus on this part of the patch only. I just reviewed it, and I think it is incorrect: We cannot assume that the CRT supports %zd, therefore, PY_FORMAT_SIZE_T has to be used. I fixed it, and removed the _Py_ prefixing, and committed the result as r68381. > Also a grep through nm output that Marc-Andre did is a good check to > run from time to time and there is no reason to have false positives. make smelly |
History | |||
---|---|---|---|
Date | User | Action | Args |
2022-04-11 14:56:43 | admin | set | github: 49100 |
2009-01-07 18:43:28 | loewis | set | status: open -> closed resolution: accepted |
2009-01-07 18:43:07 | loewis | set | messages: + msg79361 |
2009-01-07 18:32:48 | lemburg | set | messages: + msg79359 |
2009-01-07 15:49:54 | belopolsky | set | messages: + msg79344 |
2009-01-07 09:33:51 | loewis | set | messages: + msg79311 |
2009-01-06 16:27:08 | belopolsky | set | messages: + msg79271 |
2009-01-06 15:53:47 | lemburg | set | messages: + msg79267 |
2009-01-06 14:57:58 | belopolsky | set | messages: + msg79265 |
2009-01-06 13:45:44 | lemburg | set | messages: + msg79263 |
2009-01-06 13:22:02 | loewis | set | messages: + msg79259 |
2009-01-06 13:01:30 | lemburg | set | messages: + msg79254 |
2009-01-06 12:28:20 | loewis | set | messages: + msg79253 |
2009-01-06 10:30:45 | lemburg | set | messages: + msg79247 |
2009-01-06 10:28:10 | loewis | set | messages: + msg79246 |
2009-01-06 05:22:09 | belopolsky | set | nosy:
+ loewis messages: + msg79231 |
2009-01-05 23:06:25 | belopolsky | set | files:
+ issue4850.diff nosy: + lemburg messages: + msg79208 keywords: + patch |
2009-01-05 20:48:45 | belopolsky | create |