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

clean paridecl.pxd #58

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

clean paridecl.pxd #58

wants to merge 1 commit into from

Conversation

videlec
Copy link
Collaborator

@videlec videlec commented Jan 17, 2018

Remove declarations from paridecl.pxd that already appear in auto_paridecl.pxd. This was just achieved by running the following script

#! /usr/bin/env python
import re   
    
fnc = re.compile("    (int|long|void|GEN) +([A-Za-z][0-9A-Za-z_]*)\(.*\)")
    
with open('auto_paridecl.pxd') as f: 
    functions = set()
    for line in f.read().split('\n'):
        match = fnc.match(line)
        if match:
            out_type, fname = match.groups() 
            functions.add(fname)
    
f_in = open('paridecl.pxd') 
f_out = open('paridecl.pxd.rewritten', 'w')
for line in f_in.read().split('\n'):
    match = fnc.match(line)
    if match:
        out_type, fname = match.groups()
        if fname in functions:
            print('undeclare {}'.format(fname))
            continue
    f_out.write(line)
    f_out.write("\n")

f_in.close()
f_out.close()

@videlec videlec mentioned this pull request Jan 17, 2018
@jdemeyer
Copy link
Collaborator

Given that you have a script to clean things up, wouldn't it be better to run that script instead at installation time as part of the autogen process? This way, we get the best of both worlds: we don't need to mess with paridecl.pxd in the sources (Jeroen happy) and we don't have duplicate declarations in the installed paridecl.pxd file (Vincent happy).

Then we could also decide to install just one file, i.e. take

paridecl.pxd + auto_paridecl.pxd - duplicates

and put that in one file.

@videlec
Copy link
Collaborator Author

videlec commented Jan 18, 2018

I like the idea. Though the autogen process is operating before the build and not at installation (if a distinction needs to be done). We certainly want to keep the name paridecl.pxd for the file containing all relevant pari declarations. And I do not want to modify a file in the sources. For doing it concretely I can for example move the current paridecl.pxd as a template in autogen/?
Furthermore, it would be good to say more explicitely how the paridecl.pxd was generated (I believe you did it based on PARI sources of a certain version).

@jdemeyer
Copy link
Collaborator

Furthermore, it would be good to say more explicitely how the paridecl.pxd was generated

Partially automatic and partially by hand. It's copying the paridecl.h file from PARI, take the parts that we need and change from C to Cython syntax.

But I don't often update the complete file. Sometimes I just update a small part (say for example, everything related to L-functions), sometimes I add 1 function that I need. So it's a bit an organic process which is not easy to document exactly.

@videlec
Copy link
Collaborator Author

videlec commented Jan 22, 2018

The cleaning code is now integrated in the autogen/.

@jdemeyer
Copy link
Collaborator

Can you explain me why you need to change the logic in generator.py so much? Why do you need to loop twice over the PARI functions? And why does the signature of handle_pari_function have to change?

Maybe it works, but it looks overcomplicated to me.

@videlec
Copy link
Collaborator Author

videlec commented Jan 25, 2018

The problem is that there is a discrepancy between the list of functions in pari.desc and what could be generated in the Cython header paridecl.pxd. In order to do this I decided to first filter the functions from pari.desc that we will be able to treat. In this context, a function is treatable if it passes can_handle_function and parse_prototype.

@jdemeyer
Copy link
Collaborator

Wouldn't it be much simpler to just return a boolean value from handle_pari_function() to indicate whether the function was treatable? In other words, start handle_pari_function() with

        try:
            args, ret = parse_prototype(prototype, help)
        except NotImplementedError:
            return False  # Skip unsupported prototype codes

and then check the return value of handle_pari_function.

@videlec
Copy link
Collaborator Author

videlec commented Jan 30, 2018

Would also work. I wanted to avoid calling twice parse_prototype.

@videlec
Copy link
Collaborator Author

videlec commented Jan 30, 2018

done.

Though, in the future we might want to distinguish between functions for which we can actually write the definition in paridecl.pxd automatically and the one for which we are able to write a method of a Gen.

@jdemeyer
Copy link
Collaborator

done.

That's not really what I meant (I said to change handle_pari_function; that way you wouldn't need to call parse_prototype twice), but that's OK.

This should be return False though:

return  # Skip unsupported prototype codes

But you're still looping twice over all functions... the whole point of changing handle_pari_function (or can_handle_function as you did) is that this is not needed.

@videlec
Copy link
Collaborator Author

videlec commented Jan 31, 2018

Then I don't understand what you suggest.

@videlec
Copy link
Collaborator Author

videlec commented Jan 31, 2018

Ho now I see.

@jdemeyer
Copy link
Collaborator

By the way, since this changes significantly the installed .pxd files, I would like to merge this in cypari2-2.0.0 and not in cypari2-1.x.y. OK for you?

@videlec
Copy link
Collaborator Author

videlec commented Feb 1, 2018

Actually, I don't see how to avoid going through the list of functions twice. The code is doing

  1. write the template declaration file in paridecl.pxd without the handled functions (as a prerequisite we need the list of handled functions)

  2. write the auto declarated function after the template part

@jdemeyer
Copy link
Collaborator

Maybe we could simplify the generator by keeping the files in memory instead of immediately writing them out.

@jdemeyer
Copy link
Collaborator

The largest file is a bit over 1MB in size, so it shouldn't be a problem to manage it in memory.

@videlec videlec force-pushed the master branch 4 times, most recently from 28f2382 to 3660f17 Compare November 11, 2022 10:08
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

Successfully merging this pull request may close these issues.

2 participants