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

Remove unused headers from gsl/pointers #1153

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

knst
Copy link

@knst knst commented May 30, 2024

  • forward is already declared in utility, no need to include <algorithm> which is relatively heavy
  • hash is already declared in memory, no need to bring brand-new header <system_error> for hash only.

On my machine with my customized fork of GSL it improves a compilation time of gsl/pointers drastically.

See benchmark (best time over 5 runs):

  • baseline
$ time g++ -o a.out gsl/pointers.h -O2 -g -I.
real    0m0,572s
user    0m0,461s
sys     0m0,108s
  • removed algorithm:
real    0m0,505s
user    0m0,398s
sys     0m0,107s
  • without algorithm and system_error:
real    0m0,332s
user    0m0,265s
sys     0m0,067s

Considering using gsl/pointers all over codebase it speeds up compilation time of any project that uses gsl/pointers.

@beinhaerter
Copy link
Contributor

I just see that std::greater<> and std::less<> are used, but <functional> is not included for them. Maybe this can be fixed, too?

include/gsl/pointers Outdated Show resolved Hide resolved
include/gsl/pointers Outdated Show resolved Hide resolved
forward is already declared in utility, no need to include algorithm which is relativaly heavy
hash is already declared in memory, no need to bring brand-new header system_error for hash only
@knst knst force-pushed the optimize-gsl-pointers-compile-time branch from 7018630 to 138ac72 Compare May 30, 2024 12:28
@knst knst requested a review from beinhaerter May 30, 2024 12:30
@knst
Copy link
Author

knst commented Jun 25, 2024

hi! Is there any chances too see a progress here? Can it get merged to the main?

it seems useful changes in my opinion.

@d-winsor d-winsor self-assigned this Jul 17, 2024
@d-winsor
Copy link
Collaborator

@knst There appears to be a problem running checks on this PR. Can you push a dummy change as shown below to retrigger the tests.

commit -m "retrigger checks" --allow-empty

@knst knst force-pushed the optimize-gsl-pointers-compile-time branch from 138ac72 to 2445d84 Compare July 18, 2024 05:29
@knst
Copy link
Author

knst commented Jul 18, 2024

@d-winsor

3 workflows awaiting approval

problem running checks on this PR

I force pushed a branch, seems as I need approval again

@PastaPastaPasta
Copy link

@knst There appears to be a problem running checks on this PR. Can you push a dummy change as shown below to retrigger the tests.

commit -m "retrigger checks" --allow-empty

Hi @d-winsor, it appears the CI is still not triggering? Please advise.

Thanks!

@PastaPastaPasta
Copy link

Hey all,

this has been approved for a while? CI failures seem unrelated? Please advise, it'd be nice to get some movement again

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.

5 participants