Patch for new whole and dependencies
Jose E. Marchesi
jemarch@gnu.org
Tue Jul 14 08:50:19 GMT 2026
> Good evening everyone,
>
> Just before I throw the new patch over the fence, I have a few
> comments to my comments below.
>
>
> On Mon, Jul 13, 2026 at 11:26 AM chris hermansen <clhermansen@gmail.com> wrote:
>>
>> Good morning everyone,
>>
>> On Mon, Jul 13, 2026 at 3:02 AM Jose E. Marchesi <jemarch@gnu.org> wrote:
>> >
>> >
>> > Hello Chris!
>> > Thank you very much for the patch.
>> >
>> > Please find some comments inlined below. Once these get addressed,
>> > please submit a new version of the patch. It would be good if you would
>> > use a more standard Subject line for patches such as:
>>
> [stuff that doesn't need more followup at this time deleted]
>
>>
>> >
>> > > + {iter L {long long } {long } {} {short } {short short }}
>> > > + {L}int
>> > > + {reti {,}}
>> > > + ) number) int:
>> > > + begin
>> > > + long long int work =
>> >
>> > Please follow the coding conventions described in
>> > https://gcc.gnu.org/onlinedocs/ga68-coding-guidelines/.
>> >
>> > Generally speaking, when in doubt, follow the current style in the file
>> > :)
>>
>> I had thought I was but I will review and correct.
>
> Sooooooooooooooo... I have made some effort to follow the coding conventions
> but I'm not completely sure I have some (many?) things right.
>
> In particular, I'm somewhat flummoxed by what to do with lengthy
> clauses that must be
> broken onto new lines. Perhaps there are signals in the documentation
> that I'm missing.
>
> For example, is the serial clause in the "if" part correct?
>
> case v
> in
> {iter L {long long } {long } {} {short } {short short }}
> ({L}int x):
> if int digits_required = WHOLEDIGITS x;
> bool negative = x < {L}0;
> int signs_required = (negative OR width > 0 | 1 | 0);
> int chars_required = signs_required + digits_required;
> int chars_available = (width = 0 | chars_required | ABS width);
> chars_available < chars_required
> then
> chars_available * "*"
Technically speaking that would be:
if int digits_required = WHOLEDIGITS x;
bool negative = x < {L}0;
int signs_required = (negative OR width > 0 | 1 | 0);
int chars_required = signs_required + digits_required;
int chars_available = (width = 0 | chars_required | ABS width);
chars_available < chars_required
then chars_available * "*"
If you think that results into difficult to read formatting in this
particular case, it is ok to do it differently.
>
> How about this "for... while... do"?
>
> for i from (LWB whole_powers_of_10) + 1 to UPB whole_powers_of_10
> while work <= -whole_powers_of_10[i]
> do
> num_digits +:= 1
Same, in principle the `num_digits +:= 1' shall be placed after the
`do'.
> In relation to the "when in doubt, follow the current style in the
> file" advice, the body of
> the existing proc "fixed" starts with
>
> pub proc fixed = (Number v, int width, after) string:
> case v in....
>
> whereas the guidelines show:
>
> case v
> in (void): "empty",
> (bool b): (b | "true" | "false"),
> (string s): s
> esac
>
> ie the "in" follows on the next line.
That's because standard.a68.in is processed by pspp, and the
preprocessor tags {iter ...} shall be placed in their own lines:
pub proc fiaxed = (Number v, int width, after) string:
case v in
{iter L {} {long} {long long}}
> I look forward to getting the style of my contributions as close as
> possible to the guideline...
>
> As a separate topic, I would be happy to think up some further
> examples that can be added to the guidelines.
Thank you :)
This actually was already useful, as I found (and fixed) a couple of
typos in the style document.
>>
>>
>> >
>> > > + case number in
>> > > + {iter L {long long } {long } {} {short } {short short }}
>> > > + {iter K {} {LENG } {LENG LENG } {LENG LENG LENG } {LENG LENG LENG LENG }}
>> > > + ({L}int x):
>> > > + {K}(x > {L}0 | -x | x)
>> > > + {reti {,}}
>> > > + esac;
>> > > + if work > - long long 10 then 1
>> > > + elif work > - long long 100 then 2
>> > > + elif work > - long long 1 000 then 3
>> > > + elif work > - long long 10 000 then 4
>> > > + elif work > - long long 100 000 then 5
>> > > + elif work > - long long 1 000 000 then 6
>> > > + elif work > - long long 10 000 000 then 7
>> > > + elif work > - long long 100 000 000 then 8
>> > > + elif work > - long long 1 000 000 000 then 9
>> > > + elif work > - long long 10 000 000 000 then 10
>> > > + elif work > - long long 100 000 000 000 then 11
>> > > + elif work > - long long 1 000 000 000 000 then 12
>> > > + elif work > - long long 10 000 000 000 000 then 13
>> > > + elif work > - long long 100 000 000 000 000 then 14
>> > > + elif work > - long long 1 000 000 000 000 000 then 15
>> > > + elif work > - long long 10 000 000 000 000 000 then 16
>> > > + elif work > - long long 100 000 000 000 000 000 then 17
>> > > + elif work > - long long 1 000 000 000 000 000 000 then 18
>> >
>> > Will the code above work for 32-bit long long, or it relies on 64-bit
>> > (at least) long longs?
>>
>> It relies on 64 bit long longs, because they must accommode that 18
>> decimal digit number.
>>
>> The same holds true if we move to 128 bit long longs; the search must be
>> made larger.
>>
>> I'm going to think about this; I should be able to fill a row with the numbers,
>> base its size on long long, and do this in a loop instead.
>>
>> >
>> > > + else 19 fi
>> > > + end { WHOLEDIGITS };
>> > > +
>> > > + { The whole_powers_of_10 row is used to look up the appropriate power of 10
>> > > + for each integer division required to select the leading in the
>> > > conversion
>> > > + process and for each integer multiplication to eliminate the leading
>> > > + digit.
>> > > +
>> > > + Note that if long long ever becomes (say) 128 this table needs to be
>> > > + lengthened. At some point, it may be worthwhile to have a
>> > > separate lookup
>> > > + for each length (maybe even now).
>> > > + }
>> > > +
>> > > + []long long int {ℵ₀} whole_powers_of_10 = []long long int (long long 1,
>> > > + long long 10, long long 100, long long 1 000, long long 10 000,
>> > > + long long 100 000, long long 1 000 000, long long 10 000 000,
>> > > + long long 100 000 000, long long 1 000 000 000,
>> > > + long long 10 000 000 000, long long 100 000 000 000,
>> > > + long long 1 000 000 000 000, long long 10 000 000 000 000,
>> > > + long long 100 000 000 000 000, long long 1 000 000 000 000 000,
>> > > + long long 10 000 000 000 000 000, long long 100 000 000 000 000 000,
>> > > + long long 1 000 000 000 000 000 000);
>>
>> I should be able to "parametrize" this as well using the size of long long.
>>
>> [remainder of patch text deleted]
>
> I was able to get rid of the static nature of both the
> whole_powers_of_10 row and the WHOLEDIGITS operator. So now
> whole_powers_of_10 is initialized according to the size of long long
> int, and WHOLEDIGITS compares its parameter against the powers_of_10
> row, rather each having a fixed length list.
>
> One thing I ran across that had me reaching for a strong cocktail was
> that my code in my test harness ran fine but when I put it into
> standard.a68.in my tests all crashed with ugly messages like this:
>
> standard.a68:158: runtime error: bound -1940591207 out of range
> [140731252796824:1]
> Aborted (core dumped) ./a.out
>
> It turns out that when I converted whole_powers_of_10 from [] int to
> ref [] int, I needed to put the actual storage on the heap. I guess
> this makes sense, that local storage allocated within a module
> disappears by the time the module is accessed from afar.
>
> Still, I wonder if this is a thing that can be checked at compile time?
It definitely can be checked at compile time.
You could detect the size of `long long int' in libga68/configure.ac and
then have two versions of these procedures in their own files, then you
include in the compilation the file with the appropriate version.
> Thanks again for the help!
More information about the Algol68
mailing list