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 OpenDdl and StanfordImporter #30

Merged
merged 1 commit into from Apr 7, 2017

Conversation

Projects
None yet
5 participants
@alicemargatroid
Contributor

alicemargatroid commented Apr 2, 2017

Real small PR, replaces push_back with emplace_back and adds some qualifiers to improve performance

@coveralls

This comment has been minimized.

coveralls commented Apr 2, 2017

Coverage Status

Coverage remained the same at 87.238% when pulling 98618f2 on alicemargatroid:master into dfdb0ed on mosra:master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented Apr 2, 2017

Coverage Status

Coverage remained the same at 87.238% when pulling 98618f2 on alicemargatroid:master into dfdb0ed on mosra:master.

@@ -428,7 +428,7 @@ class MAGNUM_TRADE_OPENGEXIMPORTER_EXPORT Document {
MAGNUM_TRADE_OPENGEXIMPORTER_LOCAL std::size_t dereference(std::size_t originatingStructure, Containers::ArrayView<const char> reference) const;
MAGNUM_TRADE_OPENGEXIMPORTER_LOCAL bool validateLevel(std::optional<Structure> first, Containers::ArrayView<const std::pair<Int, std::pair<Int, Int>>> allowedStructures, Containers::ArrayView<const Validation::Structure> structures, std::vector<Int>& counts) const;
MAGNUM_TRADE_OPENGEXIMPORTER_LOCAL bool validateLevel(const std::optional<Structure>& first, Containers::ArrayView<const std::pair<Int, std::pair<Int, Int>>> allowedStructures, Containers::ArrayView<const Validation::Structure> structures, std::vector<Int>& counts) const;

This comment has been minimized.

@mosra

mosra Apr 7, 2017

Owner

Hmm. The Structure is just two pointers in a POD, so I thought it is completely okay to pass it via value. But the std::optional probably adds some nontrivial boilerplate around so it's not as free to copy anymore.

This comment has been minimized.

@Squareys

Squareys Apr 8, 2017

Contributor

POD? Plain old ...? 😅

This comment has been minimized.

@LB--

LB-- Apr 9, 2017

Contributor

@Squareys Plain Old Data, unless you're suggesting something else?

This comment has been minimized.

@mosra

mosra Apr 9, 2017

Owner

Data. :D

This comment has been minimized.

@alicemargatroid

alicemargatroid Apr 9, 2017

Contributor

@mosra @Squareys @LB-- The current term would be "Standard Layout"; we're working in C++11 I think.
But std::optional is not standard layout, I believe.

@mosra mosra merged commit 98618f2 into mosra:master Apr 7, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 87.238%
Details
@mosra

This comment has been minimized.

Owner

mosra commented Apr 7, 2017

Merged, thank you!

@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