[PATCH] misc cleanups

[Qian Yun]

This is a relatively large misc cleanup, so I'd like a review before
commit.

A question: shall we remove GCL_DIST related stuff in
src/scripts/mkdist.sh?

- Qian

[Qian Yun]

Follow up question: shall we move "*c_type_as_string*, c_type_as_string,
c_args_as_string, make_extern" to a ECL exclusive block?

- Qian

[Waldek Hebisch]

On Thu, Nov 16, 2023 at 08:51:40AM +0800, Qian Yun wrote:
> Follow up question: shall we move "*c_type_as_string*, c_type_as_string,
> c_args_as_string, make_extern" to a ECL exclusive block?

No. They are valid and at least potentially useful regardless of
Lisp that we use.

--
Waldek Hebisch

[Waldek Hebisch]

On Wed, Nov 15, 2023 at 06:00:26PM +0800, Qian Yun wrote:
> This is a relatively large misc cleanup, so I'd like a review before
> commit.

Looks mostly good. But I have doubts about two changes to
fricas-lisp.lisp. First, removing extern declaration of 'sock_get_float'
looks dangerous. Namely, extern declarations are needed.
We provide extern declarations for ECL in 'ecl-foreign-call',
but we do not do this for GCL. Is GCL providing extern
declarations? If not we need our own. Second, I do not
know why you want to move definition of 'sockSendString'?
It is closely related to 'sock_send_string_len' and keeping
them together looks clearer for me.

> A question: shall we remove GCL_DIST related stuff in
> src/scripts/mkdist.sh?

Probably. It was not used for many years and there is low probability
that it will be needed in the future.


--
Waldek Hebisch

[Qian Yun]

On 11/18/23 11:03, Waldek Hebisch wrote:
> On Wed, Nov 15, 2023 at 06:00:26PM +0800, Qian Yun wrote:
>> This is a relatively large misc cleanup, so I'd like a review before
>> commit.
>
> Looks mostly good. But I have doubts about two changes to
> fricas-lisp.lisp. First, removing extern declaration of 'sock_get_float'
> looks dangerous. Namely, extern declarations are needed.
> We provide extern declarations for ECL in 'ecl-foreign-call',
> but we do not do this for GCL. Is GCL providing extern
> declarations? If not we need our own. Second, I do not

The other externs were removed in commit 84fc10e4 in 2008, only this
one remains.

From GCL documentation:

In order for C code to be loaded in by load you should declare any
variables and functions to be static. If you will link them in at
build time, of course you are allowed to define new externals.

So declarations are needed when you want to load the compiled file
in interpreter. Since we are linking the .o files, this is not a
problem.

> know why you want to move definition of 'sockSendString'?
> It is closely related to 'sock_send_string_len' and keeping
> them together looks clearer for me.

It is enclosed in "foreign-defs", which is defined differently
for CMUCL and CLISP. So for consistency, I'd like to move
it out of "foreign-defs".

- Qian

[Waldek Hebisch]

On Sat, Nov 18, 2023 at 12:15:11PM +0800, Qian Yun wrote:
>
>
> On 11/18/23 11:03, Waldek Hebisch wrote:
> > On Wed, Nov 15, 2023 at 06:00:26PM +0800, Qian Yun wrote:
> > > This is a relatively large misc cleanup, so I'd like a review before
> > > commit.
> >
> > Looks mostly good. But I have doubts about two changes to
> > fricas-lisp.lisp. First, removing extern declaration of 'sock_get_float'
> > looks dangerous. Namely, extern declarations are needed.
> > We provide extern declarations for ECL in 'ecl-foreign-call',
> > but we do not do this for GCL. Is GCL providing extern
> > declarations? If not we need our own. Second, I do not
>
> The other externs were removed in commit 84fc10e4 in 2008, only this
> one remains.

AFAICS commit removed externs that we do not use and kept only used
ones.

> From GCL documentation:
>
> In order for C code to be loaded in by load you should declare any
> variables and functions to be static. If you will link them in at
> build time, of course you are allowed to define new externals.
>
> So declarations are needed when you want to load the compiled file
> in interpreter. Since we are linking the .o files, this is not a
> problem.

This really says nothing about 'extern' declarations on Lisp side.
And times have changed: old C had "implict int" rule governing
undeclared functions, but this rule for many years were considered
obsolete and now is gone from C standard. More important, C
compilers frequently are configured to reject code trying to
use undeclared functions.

So, let me reiterate: extern declarations are needed. Now they
should be present for all calls. For calls returning 'double'
even with old rules call would not work (we would silently get
wrong code). I did not add declarations earlier, as this
could be done by Lisp implementation. But ECL folks do not
want to do this, so we add them on FriCAS side. It is likely
that GCL also is not adding declarations and that we need to
add them on our side.

> > know why you want to move definition of 'sockSendString'?
> > It is closely related to 'sock_send_string_len' and keeping
> > them together looks clearer for me.
>
> It is enclosed in "foreign-defs", which is defined differently
> for CMUCL and CLISP. So for consistency, I'd like to move
> it out of "foreign-defs".

'sockSendString' can not work without 'sock_send_string_len',
so effectively its definition is complete only when "foreign-defs"
are done. And 'sockSendString' is clesely related to 'sock_send_string_len'.
So IMO "foreign-defs" is red herring here, for consistency we
should keep the two functions together.

--
Waldek Hebisch

[Qian Yun]

On 11/18/23 19:32, Waldek Hebisch wrote:
>
> This really says nothing about 'extern' declarations on Lisp side.
> And times have changed: old C had "implict int" rule governing
> undeclared functions, but this rule for many years were considered
> obsolete and now is gone from C standard. More important, C
> compilers frequently are configured to reject code trying to
> use undeclared functions.
>
> So, let me reiterate: extern declarations are needed. Now they
> should be present for all calls. For calls returning 'double'
> even with old rules call would not work (we would silently get
> wrong code). I did not add declarations earlier, as this
> could be done by Lisp implementation. But ECL folks do not
> want to do this, so we add them on FriCAS side. It is likely
> that GCL also is not adding declarations and that we need to
> add them on our side.

I can fix this in a later commit.

>>> know why you want to move definition of 'sockSendString'?
>>> It is closely related to 'sock_send_string_len' and keeping
>>> them together looks clearer for me.
>>
>> It is enclosed in "foreign-defs", which is defined differently
>> for CMUCL and CLISP. So for consistency, I'd like to move
>> it out of "foreign-defs".
>
> 'sockSendString' can not work without 'sock_send_string_len',
> so effectively its definition is complete only when "foreign-defs"
> are done. And 'sockSendString' is clesely related to 'sock_send_string_len'.
> So IMO "foreign-defs" is red herring here, for consistency we
> should keep the two functions together.
>

Fine by me. But what about |sockGetStringFrom|? Shall it be enclosed
inside "foreign-defs" as well?

I'll leave this change and commit the rest.

- Qian

[Waldek Hebisch]

On Sat, Nov 18, 2023 at 07:46:03PM +0800, Qian Yun wrote:
>
>
> On 11/18/23 19:32, Waldek Hebisch wrote:
> >
> > This really says nothing about 'extern' declarations on Lisp side.
> > And times have changed: old C had "implict int" rule governing
> > undeclared functions, but this rule for many years were considered
> > obsolete and now is gone from C standard. More important, C
> > compilers frequently are configured to reject code trying to
> > use undeclared functions.
> >
> > So, let me reiterate: extern declarations are needed. Now they
> > should be present for all calls. For calls returning 'double'
> > even with old rules call would not work (we would silently get
> > wrong code). I did not add declarations earlier, as this
> > could be done by Lisp implementation. But ECL folks do not
> > want to do this, so we add them on FriCAS side. It is likely
> > that GCL also is not adding declarations and that we need to
> > add them on our side.
>
> I can fix this in a later commit.

You mean you will keep extern declaration now and look at
the problem later? That is fine. But we should not commit
such change befor checking if it is really correct.

> > > > know why you want to move definition of 'sockSendString'?
> > > > It is closely related to 'sock_send_string_len' and keeping
> > > > them together looks clearer for me.
> > >
> > > It is enclosed in "foreign-defs", which is defined differently
> > > for CMUCL and CLISP. So for consistency, I'd like to move
> > > it out of "foreign-defs".
> >
> > 'sockSendString' can not work without 'sock_send_string_len',
> > so effectively its definition is complete only when "foreign-defs"
> > are done. And 'sockSendString' is clesely related to 'sock_send_string_len'.
> > So IMO "foreign-defs" is red herring here, for consistency we
> > should keep the two functions together.
>
> Fine by me. But what about |sockGetStringFrom|? Shall it be enclosed
> inside "foreign-defs" as well?

IMO there is no need to extend "foreign-defs" to enclose
|sockGetStringFrom|. We keep definition seqentially in the file
and it should be enough for a reader to notice that definition
are related and that there is dependence.

> I'll leave this change and commit the rest.

Well, leave out the two changes.

--
Waldek Hebisch