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

Modernize Corrade #33

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@alicemargatroid
Contributor

alicemargatroid commented Apr 1, 2017

This is a minor commit that meticulously modernizes Corrade with the following changes:

  1. No moving elements that are better simply copied
  2. push_back -> emplace_back when appropriate
  3. Use defaulted constructor/destructors, not trivial ones
  4. Use more move friendly constructors
  5. Remove unnecessary quantifiers / add ones where appropriate
  6. Range-for for all

Feel free to take what you like and leave what you don't!

@coveralls

This comment has been minimized.

coveralls commented Apr 1, 2017

Coverage Status

Coverage remained the same at 96.037% when pulling adaebc2 on alicemargatroid:master into 2550746 on mosra:master.

@@ -49,7 +49,7 @@ namespace Implementation {
}
template<class T> T BasicConfigurationValue<T>::fromString(const std::string& stringValue, ConfigurationValueFlags flags) {
std::string _stringValue = stringValue;
const std::string& _stringValue = stringValue;

This comment has been minimized.

@mosra

mosra Apr 7, 2017

Owner

Wow. Why did I ever do this? This speeds up parsing of all configuration files and command-line arguments by ∞ :D

@@ -141,7 +141,7 @@ std::vector<std::string> Arguments::environment() {
return list;
}
Arguments::Arguments(std::string prefix): _prefix{prefix + '-'} {
Arguments::Arguments(const std::string& prefix): _prefix{prefix + '-'} {

This comment has been minimized.

@mosra

mosra Apr 7, 2017

Owner

I did this because I thought the operator+(string&&, char) would avoid the copy, maybe, if there would be enough capacity in the original string (which it probably won't be, so this is a pessimization). And then I forgot the std::move() anyway. Twice. Argh.

@mosra

This comment has been minimized.

Owner

mosra commented Apr 7, 2017

I rebased the changes on master and did one small change -- I don't like using auto if the type is not immediately clear from surrounding lines (and the type is not horrendously long to type), so I put concrete types into the range-for loops.

Thanks a lot for doing this! Very appreciated.

@mosra mosra closed this Apr 7, 2017

@mosra mosra added this to the 2018.02 milestone Feb 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment