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.

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

Test with Valgrind, stop leaking, more memory safety #134

Merged
merged 7 commits into from
Mar 23, 2017
Merged

Test with Valgrind, stop leaking, more memory safety #134

merged 7 commits into from
Mar 23, 2017

Conversation

muggenhor
Copy link
Contributor

@muggenhor muggenhor commented Jan 7, 2017 •
edited
Loading

In short this change does these things:

  • Adds running of the unit tests with valgrind which can be enabled with the CMake parameter -DVALGRIND_TESTS=ON
  • Add another job on Travis which uses this
  • Reduce memory allocations
  • Don't use dynamically allocated memory for global variables
    • i.e. don't store them as global pointers, instead store them globally by-value
    • this still doesn't suffer from the mentioned static-initialisation-order fiasco, because that only applies to namespace- and class-scoped variables, not function-scoped static variables
  • Remove all uses of new in favor of make_shared in src/, except for:
    • when we're building a scoped_ptr (which lacks such a factory function)
    • the log interceptor for Boost's test driver, because that test driver assumes responsibility for deallocation of the logger
  • Make ownership a lot clearer: use references where no ownership is involved if possible, smart pointers where it is

paoloambrosio
paoloambrosio reviewed Mar 22, 2017
View reviewed changes
Copy link
Member

@paoloambrosio paoloambrosio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks very good indeed.

My only problem is with changing strings to char *s (c3fdcdc). IMO it's less readable and there aren't enough allocations to justify it.

.travis.yml Outdated

branches:
only:
- master
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to do this, but not as part of this PR. Guess it's a leftover.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, I needed that to be able to test in my own fork. I'll split it off to a separate PR

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to separate this change and implement it for AppVeyor as well. As it's a trivial change and a PR wouldn't add anything to it, I'll push the change without PR.

travis.sh
@@ -1,12 +1,17 @@
#!/bin/sh
set -e #break script on non-zero exitcode from any command
set -x #display command being executed
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't remember, I can remove it if you wish.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually let's leave it there. It would make it consistent with AppVeyor and provide value 👍

travis.sh Outdated
gem install bundler
bundle install

CTEST_OUTPUT_ON_FAILURE=ON
export CTEST_OUTPUT_ON_FAILURE

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already part of #133

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merged now

@muggenhor
Copy link
Contributor Author

My only problem is with changing strings to char *s (c3fdcdc). IMO it's less readable and there aren't enough allocations to justify it.

I initially did this because it was the leading allocation (that didn't get deallocated). That was the result of StepInfo instances never getting free'd. As for readability, I think the only place where it's decreasing that is in the toSourceString function. The rest is mostly the same. But as it's not needed anymore I'll take it out.

@muggenhor
Optionally run all tests in Valgrind
77ab25c
@muggenhor
Don't use dynamic allocation when by-value statics will do
41839e9 
This still doesn't suffer from the "static init order fiasco" because
the order of function-scoped static variables _is_ defined (i.e., when
it first comes into scope it _has_ to be initialized, either previously
or at that time).

This also ensures these objects will get destroyed at program exist,
thus removing some more memory leakage.
@muggenhor
Use make_shared<T> instead of shared_ptr<T>(new T)
850f5c7 
This makes it easier (with a simple grep '\bnew\b') to find the pointers
that really aren't tracked and as such are at risk of leaking memory.

Additionally it has some performance and exception safety benefits,
they're minor compared to the readability advantage though.
@muggenhor
Pass by-ref instead of ptr, use smart ptr for the rest
ab40e12 
Pass by reference instead of by pointer, where possible. Do this because
references must always refer to a valid object (i.e. there's no such
thing as a NULL reference), and references never imply ownership
transfers. This part of the change makes it easier to recognize a lot of
code that doesn't transfer ownership, thus reducing the amount of code
to reason about to understand ownership.

Then make all owning pointers smart pointers, to communicate (to the
human reader) that these pointers actually describe an owned-by/part-of
relationship. Note that I've used shared_ptr almost everywhere, even
when there's only a single owner. This is because while scoped_ptr would
better describe the ownership relationship, it doesn't provide a usable
interface for ownership transfer. Additionally unique_ptr, which does,
only appears in Boost at version 1.57.0.

Keeping track of all ownership through smart pointers has the
additional, and significant, advantage of removing all memory leaks
that occur when running unit tests.
@muggenhor
Return 'const void*' to avoid having to cast
ca27539
@muggenhor
Make StepInfo const correct and pass it around as const
bd10763
@muggenhor
Store a shared_ptr instead of raw to StepInfo in SingleStepMatch
d6526f9 
To be really sure we won't ever have a dangling pointer there.
@paoloambrosio paoloambrosio merged commit d6526f9 into cucumber:master Mar 23, 2017
paoloambrosio added a commit that referenced this pull request Mar 23, 2017
@paoloambrosio
Merge #134 'Test with Valgrind, stop leaking, more memory safety'
505b2a9
@muggenhor muggenhor deleted the valgrind-test branch March 24, 2017 07:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paoloambrosio paoloambrosio paoloambrosio left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@muggenhor @paoloambrosio