BUG: Various fixes to _dtype_from_pep3118

[eric-wieser]

Fixes #9053, and groundwork for #9049.

Again, probably easier to review commit-by-commit

[mhvk]

I like the solution with the stream class (though I wonder to what extent you're reinventing ply...). But I cannot say I went through this in enough detail.

[mhvk]

p.s. would be nice to have #8774 so you don't have to redefine gcd and lcm...

[eric-wieser]

#8774 actually falls back on _internals._gcd for object arrays anyway. I also think that that file is minimizing the number of things from np.core it uses, to avoid the possibility of recursion loops either at import-time or runtime.

[eric-wieser]

though I wonder to what extent you're reinventing ply

I've seen in the past that dependencies for numpy are a pain from a distribution perspective (#that-pr-about-deprecating-np.float), and it's better to do simple things ourselves.

[mhvk]

Yes, makes sense. And as said, I quite like your stream class; and the path that gets taken is at least a bit clearer than with a lex file (or a regex).

[ahaldane]

• The 'stream' cleanup commit looks nice. Perhaps consider making the class global level? I'm fine as is though.
• The three bugfix commits look good:
  • The itemsize commit is partly like in ENH: properly account for trailing padding in PEP3118 #7798 as you mentioned. I am fine merging it as you have it for now. Although, as noted there we are probably incorrectly interpreting the spec: According to spec ix should be 5 bytes, not 8. If the user wants 8 they need to do ix0i. We might leave that for ENH: properly account for trailing padding in PEP3118 #7798, since the problem is purely theoretical right now: I don't know of public python modules (including numpy) which can create buffers with 'ix' format.
  • The commit using the 'names' list to store the field order looks correct, and even seems to fix bugs we didn't come across (eg the extra test that the 1-item itemsize is unchanged).
  • The autogenerated names commit looks good and is much easier to read than the old code

I read over it well enough that it has my +1 for merging. If there are no other comments I would be happy to merge in a day or two.

[ahaldane]

All right, pulling the trigger here. Thanks @eric-wieser !

[eric-wieser]

. Although, as noted there we are probably incorrectly interpreting the spec: According to spec ix should be 5 bytes, not 8

Can you give a reference for that?

[pv]

Is it that clear? As I understand, the padding is supposed to follow typical C compiler behavior.

[eric-wieser]

@pv: Here's the problem. The c code you refer to gives 8, but struct.Struct('ix').size gives 5.

[pv]

Yes, but there is also a question on whether struct.Struct implementation is correct.

[pv]

Since also `struct.Struct('ib').size == 5`, I guess the question then is whether trailing padding must be explicit or not. The PEP does not spell this out. Python's struct module syntax is older than the PEP, so maybe that is the behavior that has priority.

[ahaldane]

Here's the reference, (copied from my comment in #7798):

The struct module says this about how format strings treat trailing padding:

To align the end of a structure to the alignment requirement of a particular type, end the format with the code for that type with a repeat count of zero.

My interpretation is that for a format like 'ix', even though the type is implicitly @ (aligned) we should only add 1 trailing padding byte. If we wanted numpy-style padding bytes the format string should instead be something like 'ix0i'.