Skip to content

fix: Move the inlined debug definitions to header#519

Merged
lan496 merged 1 commit intospglib:developfrom
LecrisUT:fix/516
Aug 6, 2024
Merged

fix: Move the inlined debug definitions to header#519
lan496 merged 1 commit intospglib:developfrom
LecrisUT:fix/516

Conversation

@LecrisUT
Copy link
Copy Markdown
Collaborator

@LecrisUT LecrisUT commented Aug 4, 2024

Switched it up a bit, basically we only care about inlining when the printing is empty, so I just moved those definitions in the header itself with the appropriate guard. When the printing is needed, than it would just create normal functions.

Closes #516

@LecrisUT LecrisUT requested a review from lan496 August 4, 2024 10:22
@codecov
Copy link
Copy Markdown

codecov bot commented Aug 4, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 25.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 83.96%. Comparing base (9cdeac6) to head (643a895).
Report is 14 commits behind head on develop.

Files with missing lines Patch % Lines
src/debug.h 25.00% 3 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff             @@
##           develop     #519      +/-   ##
===========================================
+ Coverage    83.90%   83.96%   +0.05%     
===========================================
  Files           25       26       +1     
  Lines         8185     8122      -63     
  Branches      1701     1702       +1     
===========================================
- Hits          6868     6820      -48     
+ Misses        1317     1302      -15
Flag Coverage Δ
c_api 74.85% <25.00%> (+0.06%) ⬆️
fortran_api 56.29% <0.00%> (+0.09%) ⬆️
python_api 81.39% <33.33%> (+1.03%) ⬆️
unit_tests 13.48% <0.00%> (+<0.01%) ⬆️

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 mentioned this pull request Aug 4, 2024
Merged
lan496
lan496 approved these changes Aug 5, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@lan496 lan496 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@lan496
Copy link
Copy Markdown
Member

lan496 commented Aug 5, 2024

@LecrisUT Can you rebase this PR with develop?

@LecrisUT
Move the inlined definitions to header
643a895 
Unclear how `extern inline` is supposed to work, but we only care about inlining the functions for the case where they are empty (not sure if the compiler can do it without it regardless). Using `static inline` to define the empty functions when needed (`extern inline` gave multiple definition issues)

Signed-off-by: Cristian Le <[email protected]>
@LecrisUT
Copy link
Copy Markdown
Collaborator Author

LecrisUT commented Aug 5, 2024

@LecrisUT Can you rebase this PR with develop?

Feel free to use the rebase branch button in the github UI

@lan496
Copy link
Copy Markdown
Member

lan496 commented Aug 5, 2024

Then, testing-farm:fedora-* start failing 🤔

@LecrisUT
Copy link
Copy Markdown
Collaborator Author

LecrisUT commented Aug 5, 2024

Then, testing-farm:fedora-* start failing 🤔

Yeah, reported upstream: https://status.testing-farm.io/. ETA they gave is 1-2h. Feel free to enable auto-merge and I'll come back to restart it when the problem is resolved

@lan496 lan496 enabled auto-merge August 5, 2024 10:56
@lan496 lan496 merged commit f0484ce into spglib:develop Aug 6, 2024
@LecrisUT LecrisUT deleted the fix/516 branch August 12, 2024 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lan496 lan496 lan496 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Failed to build with SPGDEBUG

2 participants

@LecrisUT @lan496