msg208616 - (view) |
Author: Zachary Ware (zach.ware) * |
Date: 2014-01-21 06:05 |
The current two-pass output for the two-pass preset causes compile errors on Windows. A sample:
..\PC\winsound.c(71): error C2133: 'winsound_PlaySound__doc__' : unknown size
Line 71 (clinic-generated docstring_prototype):
PyDoc_VAR(winsound_PlaySound__doc__);
Here's a bit more info on the error: http://msdn.microsoft.com/en-us/library/c13wk277%28v=vs.100%29.aspx
There are a few viable alternatives that I can see.
1) Give PyDoc_VAR a reasonable default size. I don't like this; it seems fragile.
2) Remove docstring_prototype as an outputtable entity. I don't like this much either, it should be a reasonable thing to do.
3) Add a new PyDoc_SIZEDVAR macro, taking name and size. Argument Clinic will know the needed size and can fill it in, but it would be largely useless for manual usage. I think this is my preferred route.
4) Instead of reusing PyDoc_VAR (or a new PyDoc_SIZEDVAR), just have Clinic output the whole expanded macro, with size; e.g. "static char winsound_PlaySound__doc__[195]". This does have the downside that any future change to docstrings that would have been a simple change to the macro would have to be done in Argument Clinic, with lots of churn in every clinicized file.
Here's a patch that implements option 3.
|
msg208617 - (view) |
Author: Larry Hastings (larry) * |
Date: 2014-01-21 06:08 |
I don't understand the problem--you didn't give me enough context. Can you attach a file that demonstrates the problem?
|
msg208618 - (view) |
Author: Zachary Ware (zach.ware) * |
Date: 2014-01-21 06:19 |
Attached is PC/winsound.c, as converted in issue20172, but using the two-pass preset output scheme.
Compiling with this file produces these errors:
..\PC\winsound.c(71): error C2133: 'winsound_PlaySound__doc__' : unknown size [P:\ath\to\cpython\PCbuild\winsound.vcxproj]
..\PC\winsound.c(79): error C2133: 'winsound_Beep__doc__' : unknown size [P:\ath\to\cpython\PCbuild\winsound.vcxproj]
..\PC\winsound.c(87): error C2133: 'winsound_MessageBeep__doc__' : unknown size [P:\ath\to\cpython\PCbuild\winsound.vcxproj]
MSVC doesn't like declaring the unsized array without an asignment.
|
msg208620 - (view) |
Author: Larry Hastings (larry) * |
Date: 2014-01-21 06:54 |
Does your proposed solution work properly when docstrings are turned off? Try undefining WITH_DOC_STRINGS. I bet you need to make the size 1 in that case.
Also, the length must be len(f.docstring) + 1, to account for the trailing \0.
Also, error C2133 doesn't look like it's applicable. On line 71 it's not being stored in a structure. Dumb MSVC.
Also PyDoc_SIZEDVAR is a bad name. PyDoc_SIZED_VAR would be an improvement, but I think PyDoc_VAR_WITH_SIZE is the name you want there.
And, I'm still not a +1 on this approach. Can you propose something else?
Finally, I will note that the "buffer" preset would work just fine in this file, if you dumped the buffer just above the PyMethodDef array.
|
msg208621 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2014-01-21 07:40 |
Why PyDoc_VAR prototype is used at all? There are only two occurrences of PyDoc_VAR in current code, and one of them actually can be replaced by PyDoc_STRVAR.
|
msg208623 - (view) |
Author: Larry Hastings (larry) * |
Date: 2014-01-21 07:49 |
Serhiy: PyDoc_VAR is used in the "two-pass" approach as a forward declaration for docstrings. Imagine if, in winsound.c, sound_methods was defined above the "dump buffer" block. The expansion of WINSOUND_PLAYSOUND_METHODDEF would include a reference to winsound_PlaySound__doc__, which hadn't been defined yet.
winsound.c is simple enough, it doesn't need the two-pass approach. But two-pass would work well for _pickle.c, where there are four or five PyMethodDef structures in the middle of the file. Part of this is just Zachary experimenting with two-pass.
|
msg208681 - (view) |
Author: Zachary Ware (zach.ware) * |
Date: 2014-01-21 18:08 |
Larry Hastings wrote:
> Does your proposed solution work properly when docstrings are turned
> off? Try undefining WITH_DOC_STRINGS. I bet you need to make the
> size 1 in that case.
For certain values of "properly", yes. It builds with no warnings or errors on Windows, but I don't know how it does on memory usage. It's easy enough to define PyDoc_VAR_WITH_SIZE alongside PyDoc_STR and use '1' instead of 'size' when WITH_DOC_STRINGS is undefined, though.
> Also, the length must be len(f.docstring) + 1, to account for the
> trailing \0.
I thought so as well, but testing both ways showed no difference. To provoke a difference, I defined length as len(f.docstring) - 1, which threw a compiler warning about an overflow. Adding 1 is probably safer, though.
> Also, error C2133 doesn't look like it's applicable. On line 71 it's
> not being stored in a structure. Dumb MSVC.
I can't argue that one :)
> Also PyDoc_SIZEDVAR is a bad name. PyDoc_SIZED_VAR would be an
> improvement, but I think PyDoc_VAR_WITH_SIZE is the name you want
>there.
Fair enough.
> And, I'm still not a +1 on this approach. Can you propose something
> else?
I gave the 4 alternatives I could think of; I'll keep thinking and throw out anything else I come up with.
> Finally, I will note that the "buffer" preset would work just fine in
> this file, if you dumped the buffer just above the PyMethodDef array.
Indeed; winsound is just a convenient testbed since it's such a small module, but the problem will affect any module that can really benefit from the two-pass output system.
|
msg208704 - (view) |
Author: Larry Hastings (larry) * |
Date: 2014-01-21 20:59 |
If in C you define
static char a[5] = "abcde"
C suppresses the trailing '\0'. That it continued to work okay was a lucky break--you must not have looked in many docstrings, or you lucked out and they happened to be padded with zeroes.
|
msg208706 - (view) |
Author: Larry Hastings (larry) * |
Date: 2014-01-21 21:08 |
I had to throw it in a struct to prevent gcc from rearranging the variables. But this demonstrates the problem--when it prints the string, it doesn't stop at the end.
-----
#include <stdio.h>
typedef struct
{
int a;
char b[8];
int c;
} abc_t;
int main(int argc, char *argv[])
{
abc_t abc = {-1, "abcdefgh", -1};
printf("abc %i%s%i\n", abc.a, abc.b, abc.c);
return 0;
}
-----
|
msg224774 - (view) |
Author: Zachary Ware (zach.ware) * |
Date: 2014-08-04 20:48 |
You're right, of course; I don't know how I got a non-screwball result in prior testing. Here's an updated patch.
I have not come up with any better alternative.
|
msg236593 - (view) |
Author: Steve Dower (steve.dower) * |
Date: 2015-02-25 16:33 |
Not sure how helpful this is, but the following code compiles fine for me (VC 12.0 and 14.0):
#include <stdio.h>
char myStr[];
struct { char* a; } myStruct = { myStr };
int main() {
printf("%s", myStruct.a);
return 0;
}
static char myStr[] = "123456789";
Any reason you can't define the forward definition like this? (It only seems to work with myStr[] and not *myStr, for some reason.)
|
msg236594 - (view) |
Author: Larry Hastings (larry) * |
Date: 2015-02-25 16:36 |
Perhaps, now that Guido allows us to redirect into a separate file, we should simply abandon the two-pass approach.
|
msg236595 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2015-02-25 16:47 |
You forgot "static" in the declaration Steve.
|
msg236596 - (view) |
Author: Zachary Ware (zach.ware) * |
Date: 2015-02-25 16:48 |
That may be for the best.
|
msg236598 - (view) |
Author: Zachary Ware (zach.ware) * |
Date: 2015-02-25 16:48 |
(My last comment was aimed at Larry's comment about abandoning two-pass...)
|
msg236599 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2015-02-25 16:51 |
What if add "const" in PyDoc_VAR?
|
msg236601 - (view) |
Author: Steve Dower (steve.dower) * |
Date: 2015-02-25 17:00 |
You need to leave static out of the forward definition and then add it in the initialization. The variable is not accessible from other object files - I checked.
"const char[]" and "static const char[]" should work the same, though I haven't tried it.
|
msg241774 - (view) |
Author: Larry Hastings (larry) * |
Date: 2015-04-22 05:23 |
Is this problem gone, now that Serhiy changed everything over to the "file" preset?
|
msg241778 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2015-04-22 06:52 |
The "file" preset is default now, but if you will consider Argument Clinic as general tool for use in third-party software, the bug still is here.
|
msg241779 - (view) |
Author: Larry Hastings (larry) * |
Date: 2015-04-22 07:09 |
I thought the bug only happened with the "two-pass" preset. The "two-pass" preset is gone, because I broke it, and nobody was using it anyway, so I removed it.
(I don't even remember what "two-pass" was trying to do. It's possible the same effect could be achieved with the modern "every destination has an arbitrary number of separate numbered buffers you can put text into" technology.)
|
msg241786 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2015-04-22 08:06 |
Ah, if the "two-pass" preset is gone, then of course the bug is gone too.
|
msg303430 - (view) |
Author: Zachary Ware (zach.ware) * |
Date: 2017-09-30 21:01 |
With two-pass gone, this is gone.
|
|
Date |
User |
Action |
Args |
2022-04-11 14:57:57 | admin | set | github: 64522 |
2017-09-30 21:01:13 | zach.ware | set | status: open -> closed resolution: remind -> out of date messages:
+ msg303430
stage: patch review -> resolved |
2015-04-22 08:06:25 | serhiy.storchaka | set | messages:
+ msg241786 |
2015-04-22 07:09:08 | larry | set | messages:
+ msg241779 |
2015-04-22 06:55:28 | serhiy.storchaka | set | resolution: remind versions:
+ Python 3.6, - Python 3.4, Python 3.5 |
2015-04-22 06:52:58 | serhiy.storchaka | set | priority: normal -> low
messages:
+ msg241778 |
2015-04-22 05:23:15 | larry | set | messages:
+ msg241774 |
2015-02-25 17:00:13 | steve.dower | set | messages:
+ msg236601 |
2015-02-25 16:51:45 | serhiy.storchaka | set | messages:
+ msg236599 |
2015-02-25 16:48:50 | zach.ware | set | messages:
+ msg236598 |
2015-02-25 16:48:06 | zach.ware | set | messages:
+ msg236596 |
2015-02-25 16:47:35 | serhiy.storchaka | set | messages:
+ msg236595 |
2015-02-25 16:36:21 | larry | set | messages:
+ msg236594 |
2015-02-25 16:34:00 | steve.dower | set | messages:
+ msg236593 |
2015-02-25 15:28:32 | serhiy.storchaka | set | nosy:
+ tim.golden, steve.dower components:
+ Argument Clinic
|
2014-08-04 20:48:12 | zach.ware | set | files:
+ issue20323.v2.diff
messages:
+ msg224774 stage: patch review |
2014-08-03 00:01:20 | BreamoreBoy | set | versions:
+ Python 3.5 |
2014-01-21 21:08:02 | larry | set | messages:
+ msg208706 |
2014-01-21 20:59:49 | larry | set | messages:
+ msg208704 |
2014-01-21 18:08:29 | zach.ware | set | messages:
+ msg208681 |
2014-01-21 07:49:51 | larry | set | messages:
+ msg208623 |
2014-01-21 07:40:09 | serhiy.storchaka | set | nosy:
+ serhiy.storchaka messages:
+ msg208621
|
2014-01-21 06:54:01 | larry | set | messages:
+ msg208620 |
2014-01-21 06:19:51 | zach.ware | set | files:
+ winsound.c
messages:
+ msg208618 |
2014-01-21 06:08:52 | larry | set | messages:
+ msg208617 |
2014-01-21 06:06:37 | zach.ware | set | components:
+ Build, Demos and Tools, Windows versions:
+ Python 3.4 |
2014-01-21 06:05:43 | zach.ware | create | |