Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Autogenerate headers #55

Closed
videlec opened this issue Jan 15, 2018 · 19 comments
Closed

Autogenerate headers #55

videlec opened this issue Jan 15, 2018 · 19 comments

Comments

@videlec
Copy link
Collaborator

videlec commented Jan 15, 2018

As discussed in #28 and revived in Sage trac ticket #24531 we discuss the possibility of (partially or completely) auto generating paridecl.pxd from (one or both of) pari.desc or paridecl.h.

@jdemeyer
Copy link
Collaborator

Does #56 solve your problem?

@videlec
Copy link
Collaborator Author

videlec commented Jan 15, 2018

It gets better. Why should we keep such a big paridecl.pxd if almost everything is already inside auto_paridecl.pxd? Making it minimal, we wouldn't have to worry about signature changes in upstream.

@jdemeyer
Copy link
Collaborator

jdemeyer commented Jan 16, 2018

almost everything is already inside auto_paridecl.pxd

This statement is simply not true: auto_paridecl.pxd contains only declarations for functions which are accessible from GP. There are a lot more functions in PARI which are not directly exposed to GP. Those are only in paridecl.pxd.

Making it minimal, we wouldn't have to worry about signature changes in upstream.

With #56 we don't have to worry either: the declarations from auto_paridecl.pxd override the ones from paridecl.pxd

@jdemeyer
Copy link
Collaborator

@videlec Can you explain better what you have in mind? Are you thinking about a script to remove duplicate declarations...?

@videlec
Copy link
Collaborator Author

videlec commented Jan 17, 2018

One of

  • use pari.h header
  • manually remove as much as we can from paridecl.pxd. If there is no duplicate with the current auto_paridecl.pxd it should be a small list.

@jdemeyer
Copy link
Collaborator

jdemeyer commented Jan 17, 2018

manually

Really? Do you volunteer for that? It's a waste of time for zero gain.

@jdemeyer
Copy link
Collaborator

Also, there are two further issues with your proposal:

  1. It makes it harder to maintain paridecl.pxd. The manual removal would need to be done every time that we update that file.

  2. The contents of auto_paridecl.pxd depend on the PARI version, while paridecl.pxd does not. So if there is a function which appears in paridecl.pxd and it appears in auto_paridecl.pxd only for some PARI versions, what do we do?

@videlec
Copy link
Collaborator Author

videlec commented Jan 17, 2018

  1. It makes it harder to maintain paridecl.pxd. The manual removal would need
    to be done every time that we update that file.

  2. The contents of auto_paridecl.pxd depend on the PARI version, while
    paridecl.pxd does not. So if there is a function which appears in paridecl.pxd
    and it appears in auto_paridecl.pxd only for some PARI versions, what do we do?

The main point is to make paridecl.pxd lighter now. I never claimed that it needs to be up to date with the PARI/GP headers.

@jdemeyer
Copy link
Collaborator

The main point is to make paridecl.pxd lighter now.

But why? That does not fix any problem but it does create new problems. So it's a negative move for me.

@jdemeyer
Copy link
Collaborator

I never claimed that it needs to be up to date with the PARI/GP headers.

Ideally, it should be reasonably up-to-date. Now and then, it will need to be updated.

@defeo
Copy link
Member

defeo commented Jan 17, 2018

I never claimed that it needs to be up to date with the PARI/GP headers.

Ideally, it should be reasonably up-to-date. Now and then, it will need to be updated.

This seems to be the main point of disagreement. Jeroen wants to keep updating paridecl.pxd manually, a task which would render the cleanup of paridecl.pxd tedious.

Vincent wants to auto-generate paridecl from the C headers. I'm not going to enter in the merit of this proposal (my opinion is that this is reasonable, even without a proper C parser). I just want to give my 2 cents by saying that, as long as we do not autogenerate it, it seems wasteful to do a manual cleanup.

Better to have the largest possible paridecl.pxd, easy to semi-automanually produce and covering as many versions as possible, than to do some tedious work that risks breaking things for some users.

@videlec
Copy link
Collaborator Author

videlec commented Jan 17, 2018

I am not discussing now how auto_paridecl.pxd is generated but what should be the respective role of paridecl.pxd and auto_paridecl.pxd.

Different pari versions have different functions (and possible same functions with different signatures). This is the very reason of the existence of auto_paridecl.pxd. My wish is to minimize paridecl.pxd because after #56 most of the declarations are actually overriden by auto_paridecl.pxd (and you have to read the last line of paridecl.pxd to understand that).

  • The current situation is confusing for people using cypari2 as there are conflicting function declarations in paridecl.pxd and auto_paridecl.pxd.
  • Having smaller files for the very same result is good.

@defeo
Copy link
Member

defeo commented Jan 17, 2018 via email

@videlec
Copy link
Collaborator Author

videlec commented Jan 17, 2018

The current situation is confusing for people using cypari2 as there
are conflicting function declarations in paridecl.pxd and auto_paridecl.pxd.

But that's unavoidable (in principle), if we want to support many
different PARI version at once.

It is avoidable! All functions that depend on PARI versions are in auto_paridecl.pxd which is generated on the fly. We currently have wrong function declarations in paridecl.pxd overriden by auto_paridecl.pxd because of the header inclusion.

@jdemeyer
Copy link
Collaborator

All functions that depend on PARI versions are in auto_paridecl.pxd which is generated on the fly.

As I have commented above, this statement is completely false. This is probably the main source of confusion.

@videlec
Copy link
Collaborator Author

videlec commented Jan 17, 2018

All functions that depend on PARI versions are in auto_paridecl.pxd which is generated on the fly.

As I have commented above, this statement is completely false. This is probably the main source of confusion.

You might have misread (at least your reference to the above comment is unrelated). I never claimed that we can rid of paridecl.pxd. Just that (currently) the functions that do change between PARI versions are taken care of in auto_paridecl.pxd.

@defeo
Copy link
Member

defeo commented Jan 17, 2018 via email

@defeo
Copy link
Member

defeo commented Jan 17, 2018 via email

@videlec
Copy link
Collaborator Author

videlec commented Jan 17, 2018

See #58

@jdemeyer jdemeyer closed this as completed Feb 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants