Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#18778 closed enhancement (fixed)

Clean up GSL declarations

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-6.8
Component: cython Keywords:
Cc: Merged in:
Authors: Jeroen Demeyer Reviewers: François Bissey
Report Upstream: N/A Work issues:
Branch: 6740862 (Commits) Commit:
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

  1. Move all .pxi files from src/sage/gsl to .pxd files in new directory src/sage/libs/gsl.
  2. Split off all declarations for common types in src/sage/libs/gsl/types.pxd
  3. Move compile arguments from src/module_list.py to the new .pxd files.

Change History (13)

comment:1 Changed 6 years ago by jdemeyer

  • Summary changed from Move GSL pxi files to pxd files in libs/gsl to Clean up GSL declarations

comment:2 Changed 6 years ago by jdemeyer

  • Branch set to u/jdemeyer/move_gsl_pxi_files_to_pxd_files_in_libs_gsl

comment:3 Changed 6 years ago by jdemeyer

  • Commit set to 6740862036cff7d93756315929ead4461526dd6c
  • Status changed from new to needs_review

New commits:

6740862Clean up GSL includes

comment:4 Changed 6 years ago by jdemeyer

  • Description modified (diff)

comment:5 follow-ups: Changed 6 years ago by fbissey

Hum so in the case of sage/rings/complex_double the dependency on pari and gsl are now automatically added from the fact you import or declare some symbols in the corresponding .pxd file? Of course that's how the linking to pari must have been done in the first place since there was already no pari in libraries before your change. And thank you for demonstrating aliases, I feel I will abuse this in the future.

Looks good but I should at least do a build of that one because it is a lot of changes even if they are seemingly trivial.

comment:6 in reply to: ↑ 5 ; follow-up: Changed 6 years ago by jdemeyer

Replying to fbissey:

Hum so in the case of sage/rings/complex_double the dependency on pari and gsl are now automatically added from the fact you import or declare some symbols in the corresponding .pxd file?

If you replace "import or declare" by "cimport", yes, you are right. You just need to add the libraries to the .pxd file.

Of course that's how the linking to pari must have been done in the first place since there was already no pari in libraries before your change.

That was done in #18450.

And thank you for demonstrating aliases, I feel I will abuse this in the future.

Abuse? :-)

Anyway, for #17630, you'll probably want to define an analogous BLAS_LIBRARIES or something.

comment:7 in reply to: ↑ 6 Changed 6 years ago by fbissey

Replying to jdemeyer:

Anyway, for #17630, you'll probably want to define an analogous BLAS_LIBRARIES or something.

You read my mind. :)

comment:8 in reply to: ↑ 5 ; follow-up: Changed 6 years ago by jdemeyer

Replying to fbissey:

Looks good but I should at least do a build of that one because it is a lot of changes even if they are seemingly trivial.

On my Linux laptop, make ptest passes. I'll try on an OS X box also.

comment:9 in reply to: ↑ 8 Changed 6 years ago by fbissey

Replying to jdemeyer:

Replying to fbissey:

Looks good but I should at least do a build of that one because it is a lot of changes even if they are seemingly trivial.

On my Linux laptop, make ptest passes. I'll try on an OS X box also.

I don't doubt that you have done so already. I just do what I think I should do as a reviewer, running my own independent tests in this case.

comment:10 Changed 6 years ago by jdemeyer

Everything fine on OS X also.

comment:11 Changed 6 years ago by fbissey

  • Reviewers set to François Bissey
  • Status changed from needs_review to positive_review

Ready to go ahead. Have a good day.

comment:12 Changed 6 years ago by vbraun

  • Branch changed from u/jdemeyer/move_gsl_pxi_files_to_pxd_files_in_libs_gsl to 6740862036cff7d93756315929ead4461526dd6c
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:13 Changed 6 years ago by jdemeyer

  • Commit 6740862036cff7d93756315929ead4461526dd6c deleted

Thanks for the review!

Note: See TracTickets for help on using tickets.