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

Utility: use Corrade String and StringView in ConfigurationValue #146

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hugoam
Copy link

@hugoam hugoam commented Aug 10, 2022

Hello !
We're trying to optimize our compile times and I noticed that StlForwardString.h is actually very heavy on MSVC because it just directly includes <string>, so I thought let's go and try to clean up ConfigurationValue.
I'll open the associated Magnum PR right away. Let me know if I'm going at this correctly 😅

@hugoam hugoam force-pushed the configuration-value-string-cleanup branch from ac25b38 to 7c05208 Compare August 10, 2022 10:41
@codecov
Copy link

codecov bot commented Aug 10, 2022

Codecov Report

Merging #146 (7c05208) into master (6386a97) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #146      +/-   ##
==========================================
+ Coverage   97.92%   97.94%   +0.01%     
==========================================
  Files         132      133       +1     
  Lines       10749    10746       -3     
==========================================
- Hits        10526    10525       -1     
+ Misses        223      221       -2     
Impacted Files Coverage Δ
src/Corrade/Utility/Arguments.h 100.00% <ø> (ø)
src/Corrade/Utility/ConfigurationValue.h 100.00% <ø> (ø)
src/Corrade/Utility/ConfigurationValue.cpp 98.14% <100.00%> (+3.32%) ⬆️
src/Corrade/Utility/ConfigurationValueStl.h 100.00% <100.00%> (ø)
src/Corrade/Utility/Arguments.cpp 100.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@mosra mosra added this to the 2022.0a milestone Aug 17, 2022
@mosra
Copy link
Owner

mosra commented Aug 17, 2022

Hi and sorry for a late reply -- I've been away last week and only now catching up to everything.

This makes sense in general, and especially the new ConfigurationValueStl.h header does. That's how I'd do it myself as well. But the reason I neglected this part was because all APIs that actually used ConfigurationValue use std::string themselves heavily, so until I clean those up I didn't really see how fixing it just here would improve anything. Can you say where it helped in your case?

Oh, or is it just due to Mesh.h, PixelFormat.h and VertexFormat.h dragging <string> in (on MSVC), according to what I see in mosra/magnum#582? Because there it relies just on a forward declaration of ConfigurationValue and could thus be changed to String / StringView without needing to modify anything else. The Math/ConfigurationValue.h is a separate header that isn't pulled in by anything else, so it shouldn't affect your build times.

(For the rest, I feel it wouldn't really improve anything until Utility::Arguments and Utility::Configuration are de-STL-string-ified first. So I'd keep these PRs around until I get to cleaning that up.)

@hugoam
Copy link
Author

hugoam commented Aug 17, 2022

Oh, or is it just due to Mesh.h, PixelFormat.h and VertexFormat.h dragging in (on MSVC)

Yes, exactly, that was the low hanging fruit I was trying to tackle to improve compile times as those are some pretty commonly included headers, and have them drag in <string> induces a pretty big cost. So yep downsizing those PRs to just changing those 3 headers would be perfectly fine for us ! Unless obviously there is something else that we commonly include that I missed

@mosra
Copy link
Owner

mosra commented Aug 18, 2022

Done! Leaving the PRs open, will merge the rest once I get to cleaning up Utility::Arguments and Utility::Configuration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: TODO
Development

Successfully merging this pull request may close these issues.

2 participants