10000 feat: Mark C functions as deprecated by LecrisUT · Pull Request #435 · spglib/spglib · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

feat: Mark C functions as deprecated #435

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Mar 2, 2024
Merged

feat: Mark C functions as deprecated #435

merged 2 commits into from
Mar 2, 2024

Conversation

LecrisUT
Copy link
Collaborator
@LecrisUT LecrisUT commented Feb 15, 2024 •
edited
Loading

I could use some help marking which functions are to be deprecated.

  • Which functions should we deprecate. Afaiu the ones that internally call the spg_get_dataset and just extract the values from there, any other ones?
  • Should we officially deprecated OpenMP support?
  • Check python interface that everything is marked with appropriate deprecation
  • Fortran interface. Nothing we can do there 🤷

One annoying aspect, when we compile ourselves, we always get deprecation warnings, I'll think of a clever way to get around that. I am wrong, the compiler is actually clever enough to not trigger when compiling self. The warning occur because these are used in test and python interface

Closes #409

@LecrisUT LecrisUT added the deprecation Changes that will be deprecated in next release label Feb 15, 2024
@LecrisUT LecrisUT added this to the 2.5 milestone Feb 15, 2024
@LecrisUT LecrisUT mentioned this pull request Feb 15, 2024
Closed
5 tasks
@LecrisUT LecrisUT force-pushed the feat/deprecate branch 2 times, most recently from c1f6b62 to 3320a6c Compare February 15, 2024 13:19
@LecrisUT
Copy link
Collaborator Author

Interesting chicken-and-egg prroblem __has_c_attribute is not defined until C23, but then you don't have a mechanism to check for [[deprecated]]. Good thing these are both added in C23, so we can check for both at same time.

@LecrisUT LecrisUT force-pushed the feat/deprecate branch 3 times, most recently from 8475f5a to 05d490b Compare February 15, 2024 13:35
@LecrisUT LecrisUT requested review from lan496 and atztogo February 15, 2024 13:36
@LecrisUT LecrisUT modified the milestones: 2.5, 2.4 Feb 15, 2024
@codecov Codecov
Copy link
codecov bot commented Feb 15, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.59%. Comparing base (9ab66b3) to head (974151a).

Additional details and impacted files 
@@           Coverage Diff            @@
##           develop     #435   +/-   ##
========================================
  Coverage    83.59%   83.59%           
========================================
  Files           24       24           
  Lines         8118     8118           
  Branches      1689     1693    +4     
========================================
  Hits          6786     6786           
  Misses        1332     1332
Flag Coverage Δ
c_api 74.43% <ø> (ø)
fortran_api 56.16% <ø> (ø)
python_api 79.70% <ø> (ø)
unit_tests 13.47% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@LecrisUT LecrisUT marked this pull request as ready for review February 15, 2024 13:47
@LecrisUT LecrisUT force-pushed the feat/deprecate branch 2 times, most recently from b10b673 to 3a5d4ef Compare February 15, 2024 14:22
@LecrisUT LecrisUT added the enhancement Significant ehancements label Feb 15, 2024
@LecrisUT LecrisUT force-pushed the feat/deprecate branch 2 times, most recently from 59f2ef9 to ef2096b Compare February 15, 2024 21:54
@atztogo
Copy link
Collaborator
atztogo commented Feb 16, 2024

@LecrisUT, I didn't know about those features in C23. They are simply ignored in older C standard?

@LecrisUT
Copy link
Collaborator Author
LecrisUT commented Feb 16, 2024 •
edited
Loading

@LecrisUT, I didn't know about those features in C23. They are simply ignored in older C standard?

Actually they have been available for quite sometime in the major compilers, check the fallthrough, it covers gnu, clang, and microsoft. Incidentally we can check that they are functioning by looking at the build log of each compiler since we are using those deprecated functions to cover in the test-suite (I should patch it to disable the warnings at some point).

Edit: actually microsoft deprecation is not triggered 🤷

@LecrisUT
Copy link
Collaborator Author

Btw, the macros are quite hard to read, wdyt of imposing an indentation to it:
https://clang.llvm.org/docs/ClangFormatStyleOptions.html#indentppdirectives

I like the BeforeHash format.

@atztogo
Copy link
Collaborator
atztogo commented Feb 16, 2024

Btw, the macros are quite hard to read, wdyt of imposing an indentation to it:

I didn't know that we can indent the macro line. I have no objections and any convention is fine to me.

@LecrisUT
Copy link
Collaborator Author

Btw, the macros are quite hard to read, wdyt of imposing an indentation to it:

I didn't know that we can indent the macro line. I have no objections and any convention is fine to me.

@lan496 Preference?

@LecrisUT LecrisUT mentioned this pull request Feb 16, 2024
Merged
@lan496
Copy link
Member
lan496 commented Feb 16, 2024

Let's use PPDIS_BeforeHash!

lan496
lan496 reviewed Feb 16, 2024
View reviewed changes
@LecrisUT
Copy link
Collaborator Author

Note that I only marked random functions. Please go through the others as well to decide if it should be depricated or not

@LecrisUT
Copy link
Collaborator Author

I think I've marked all of the interfaces that can be easily deprecated. In general many of the interfaces should be reworked to reuse the dataset and have more nested interface.

  • spg_standardize_cell, spg_find_primitive, etc. should be converted to getters of SpglibDataset
  • Reduce the interface where you pass lattice/position/rotation/translation directly
  • ...get_symmetry_with_collinear_spin should also be reworked so that it can re-use a pre-calculated dataset "object". Will be easier to do that with a cpp interface, and making the C interface go through a void* pointer
  • spg_get_symmetry_from_database feels like it should have its own datatype from which SpglibDataset expands
  • I am not sure about the kpoint grid interface

@LecrisUT LecrisUT requested a review from lan496 February 28, 2024 17:48
LecrisUT added 2 commits March 2, 2024 13:06
@LecrisUT
Add deprecation macros
bce6150 
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
@LecrisUT
Mark C functions as deprecated
974151a 
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
@LecrisUT LecrisUT merged commit a66056a into spglib:develop Mar 2, 2024
@LecrisUT LecrisUT deleted the feat/deprecate branch March 27, 2024 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@atztogo atztogo atztogo approved these changes

@lan496 lan496 lan496 approved these changes

Assignees

@atztogo atztogo

@lan496 lan496

Labels
deprecation Changes that will be deprecated in next release enhancement Significant ehancements
Projects
Spglib developers' kanban
Status: ✅ Done
Milestone
2.4
Development

Successfully merging this pull request may close these issues.

Mark C library deprecated functions
3 participants
@LecrisUT @atztogo @lan496
0