On Thu, Jan 4, 2018 at 4:21 AM, Jerry DeLisle <jvdeli...@charter.net> wrote: > On 01/03/2018 03:37 AM, Janne Blomqvist wrote: >> On Sat, Dec 30, 2017 at 10:58 PM, Jerry DeLisle <jvdeli...@charter.net> >> wrote: >>> On 12/30/2017 12:35 PM, Janne Blomqvist wrote: >>>> On Sat, Dec 30, 2017 at 7:16 PM, Thomas Koenig <tkoe...@netcologne.de> >>>> wrote: >>> ---snip--- >>>> >>>> I can provide that stuff as a separate patch, or merge it into the >>>> original megapatch and resubmit that, whichever way you prefer. >>> >>> I would prefer we split into two patches. This will make review of the >>> library >>> I/O changes easier. The int len is used in a lot of places also where it >>> really >>> happens to also be the kind (which is a length in our implementation). >> >> Hi, >> >> attached is a patch that makes the two attached testcases work. It >> applies on top of the charlen->size_t patch. In the formatted I/O >> stuff, I have mostly used ptrdiff_t to avoid having to deal with >> signed/unsigned issues, as the previous code was using int. >> >> >> > > Well I have started reviewing. One concern I have is that in many places you > are changing formatted field widths, like w in read_l, to ptrdiff_t. I don't > think this is necessary. If someone is printing a value that has a field width > that big, it will never finish. > > I did not check the definitions of format data structures that use these > values > all over. So I think in the least, the patch goes too far.
I don't expect to support format widths larger than INT_MAX. The reason why ptrdiff_t is used is that read_block_form_* and many similar functions take a pointer to a ptrdiff_t (to handle the one case where a scalar variable can be wider than INT_MAX, namely a character), and we can't pass a pointer to an incompatible type. Now, you could argue that the internal API should be refactored so that we pass integers by value rather than as pointers, and then only the character case would need to care to use a larger type and the rest could keep using plain int. And I would agree that would be good, but I think such a refactor would be stage 1 material, not stage 3. > Another issue I have is that the readability of the code just sucks to me. The > type ptrdiff_t is so not intuitive in a very many places. If this is really > necessary lets hide it behind a macro or something so I don't have to cringe > every time I look at this code. (such as gfc_int) > > Sometimes we can get too pedantic about things like this. A lot of these > variables have nothing to do with pointers, though they may be used as > modifiers > to offsets in large memory spaces. Well, ptrdiff_t is one of the two "pointer-sized integer" typedefs introduced in C89, the other being size_t. So I would argue that if one is doing stuff like pointer arithmetic, or calculating array indices etc., and the variable might for some reason be negative, ptrdiff_t is the natural type to use. I do agree that ptrdiff_t is a bit of a mouthful, but I don't have any really good alternatives either; I'm not convinced that a libgfortran-specific typedef would make things clearer. Or we can blame Microsoft which made win64 LLP64 and not LP64 like the rest of the world, and thus spoiled the use of long for portable code. :) FWIW, we have "index_type" which is a typedef for ptrdiff_t, but that's used mostly in code that deals with array descriptors, so I thought it would be cleaner to not use it here. -- Janne Blomqvist
