Skip to content
This repository was archived by the owner on Oct 9, 2019. It is now read-only.

Remove boost dependency #19

Open
wants to merge 23 commits into
base: master
Choose a base branch
from

Conversation

kylealanhale
Copy link

The primary purpose of this pull request is to remove the boost regex dependency and use C++11’s regex support instead, as well as providing some custom trim functions to replace boost’s.

Additionally, some refactoring and logic work were done to handle edge cases, especially in the way that multi-line sections and newlines are treated that differ from the ruby implementation. Tests were added for these scenarios.

The travis config was updated to run jobs for both GCC 4.9 and Clang, and both are passing, although GCC’s implementation slightly differed, as you can see in 1304486.

A new Xcode project was also added, with unit tests wired up and passing.

@mrtazz
Copy link
Owner

mrtazz commented Jun 25, 2015

Oh wow, this is great! Thank you so much for taking the time to contribute! It will take me some time to look it over but I'm really happy about this.

@shiva
Copy link

shiva commented Jul 7, 2015

@kylealanhale Didn't notice that you had already raised these for review! Awesome.

@mrtazz Nice that you have a Travis CI build going for this. It would be great when these changes go in, and I can abandon my fork!

@shiva
Copy link

shiva commented Jul 25, 2015

@mrtazz Wondering if you have had a chance to consider merging this patch? I'm curious about some of the other branches, and support for CMake based builds, and some other features like "the fix for partial file load" etc.

export CXXFLAGS="-std=c++11";
elif [ "$CXX" = "clang++" ]; then
export CXXFLAGS="-std=c++11 -stdlib=libc++ -DGTEST_USE_OWN_TR1_TUPLE=1";
fi
Copy link
Owner

Choose a reason for hiding this comment

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

can we move those into the Makefile and the gtest build utils script? Or is this a specific travis problem? I'd much rather have it close to the actual build if this will be a problem outside of Travis.

Copy link
Author

Choose a reason for hiding this comment

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

These are for the sake of Travis, although they're good clues on dependencies needed for building anywhere. However, I believe you have the -DGTEST_USE_OWN_TR1_TUPLE=1 documented already for clang, and I think I added something about gcc-4.9 and c++11, but I'll double check and make sure it's all there somewhere.

if (!boost::regex_search(start, end, matches, section,
boost::match_default | boost::format_all))
if (!std::regex_search(start, end, matches, section,
std::regex_constants::match_default | std::regex_constants::format_default))
Copy link
Owner

Choose a reason for hiding this comment

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

newline and indent to fit in 80 chars.

@mrtazz
Copy link
Owner

mrtazz commented Jul 26, 2015

@kylealanhale I left some comments there. Most of it was really just stylistic as I think the code overall looks good. It would be great if you could in general try to make it break lines at 80 chars and remove trailing whitespace as it makes it easier for the next person to contribute regardless of editor/IDE. I also realized that this could all be made explicit in a CONTRIBUTING.md so I'll make sure to add one.

@shiva
Copy link

shiva commented Jul 28, 2015

@kylealanhale Would you prefer that I work on the comments provided @mrtazz ? Let me know if you are otherwise pre-occupied, and I will find some time for these changes.

@kylealanhale
Copy link
Author

@shiva I should be able to knock these out in the next couple of days.

@kylealanhale
Copy link
Author

Took care of those; let me know if you see anything else. Otherwise, I think this is ready to go.

@kylealanhale
Copy link
Author

What's your versioning approach? Since this is a substantial change that will break for people who for any reason can't compile with c++11 support, it might warrant a new release. And maybe a 0.3.x release before merging this to catch any recent changes, which might be useful for anyone that wants to use the old Boost version.

@mrtazz
Copy link
Owner

mrtazz commented Aug 1, 2015

@kylealanhale yea that's a good point. I wanted to make it a new release anyways. I'll check what we have in master since the last release and will tag one before merging. Thanks again for doing all this work!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants