Skip to content

Remove unused headers from gsl/pointers #1153

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
carsonRadtke merged 3 commits into from
Oct 14, 2024
Merged

Remove unused headers from gsl/pointers #1153

carsonRadtke merged 3 commits into from
Oct 14, 2024

Conversation

@knst
Copy link
Contributor

@knst knst commented May 30, 2024 •
edited
Loading

  • 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

beinhaerter commented May 30, 2024

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

beinhaerter
beinhaerter reviewed May 30, 2024
View reviewed changes
@knst
Remove unused headers from gsl/pointers
ead902b 
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 knst mentioned this pull request Jun 3, 2024
Merged
5 tasks
@knst
Copy link
Contributor 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
Contributor

d-winsor commented Jul 17, 2024

@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
Contributor 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

PastaPastaPasta commented Jul 19, 2024

@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

PastaPastaPasta commented Aug 29, 2024

Hey all,

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

@carsonRadtke carsonRadtke merged commit 84b2ca1 into microsoft:main Oct 14, 2024
@carsonRadtke
Copy link
Member

carsonRadtke commented Oct 14, 2024

@knst Thanks for the contribution!

Apologies it took so long to get the CI issues sorted out.

@knst
Copy link
Contributor Author

knst commented Oct 15, 2024

@carsonRadtke thanks a lot for getting it merged!

@knst knst deleted the optimize-gsl-pointers-compile-time branch October 15, 2024 03:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gdr-at-ms gdr-at-ms gdr-at-ms approved these changes

+2 more reviewers

@d-winsor d-winsor d-winsor approved these changes

@beinhaerter beinhaerter beinhaerter approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@d-winsor d-winsor

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@knst @beinhaerter @d-winsor @PastaPastaPasta @carsonRadtke @gdr-at-ms