Skip to content

[ML] Replace boost functions by their std equivalents #508

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

Merged
merged 7 commits into from
Jun 24, 2019

Conversation

valeriy42
Copy link
Contributor

@valeriy42 valeriy42 commented Jun 19, 2019

Following boost functions have been raplaced with their std equivalents: boost::array, boost::begin, boost::end, boost::is_pod, boost::ref, boost::cref, boost::reference_wrapper, boost::make_unique, boost::algorithm::is_sorted, boost::bind.

Due to the limitations of the std::ref implementation, boost::ref cannot be removed where it is used in Input and Output filters (e.g. lib/core/CCompressOStream.cc and lib/core/CStateDecompressor.cc).

Fixes #337

@valeriy42
Copy link
Contributor Author

recheck

@ghost
Copy link

ghost commented Jun 19, 2019

Hi @MLocs, we have found your signature in our records, but it seems like you have signed with a different e-mail than the one used in your Git commit. Can you please add both of these e-mails into your Github profile (they can be hidden), so we can match your e-mails to your Github profile?

Copy link
Contributor

@droberts195 droberts195 left a comment

Choose a reason for hiding this comment

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

Thanks for doing this! It's a lot of files to change but certainly makes the code look much more modern.

I found a few more files that could probably also be changed:

bash-3.2$ find . -type f | xargs egrep 'boost::(array|begin|end|is_pod|ref|cref|make_unique|algorithm::is_sorted|bind)'
./lib/core/unittest/CBase64FilterTest.cc:        filter.push(boost::ref(sink));
./lib/core/unittest/CBase64FilterTest.cc:        filter.push(boost::ref(source));
./lib/core/unittest/CBase64FilterTest.cc:            filter.push(boost::ref(sink));
./lib/core/unittest/CBase64FilterTest.cc:            filter.push(boost::ref(sink));
./lib/core/unittest/CBase64FilterTest.cc:        filter.push(boost::ref(source));
./lib/core/unittest/CBase64FilterTest.cc:        filter.push(boost::ref(source));
./lib/core/CCompressOStream.cc:    m_OutFilter.push(boost::ref(m_FilterSink));
./lib/core/CStateDecompressor.cc:    m_InFilter->push(boost::ref(m_FilterSource));
./lib/api/CLengthEncodedInputParser.cc:            // caused by boost::reference_wrapper not having a default
./lib/api/CLengthEncodedInputParser.cc:            // some type of vector of boost::reference_wrappers.

Also, if you run this command from the top level of the ml-cpp repo it shows lots of places where the Boost headers are still included:

find . -type f | xargs egrep 'boost/(ref|make_unique|bind).hpp'

*/

#ifndef INCLUDE_CORE_UNWRAPREF_H_
#define INCLUDE_CORE_UNWRAPREF_H_
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: following the pattern of all our other guards this would be INCLUDED_ml_core_UnwrapRef_h

(Note especially INCLUDED ending with D.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


#include <functional>

namespace ml {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please can you add a Doxygen comment. Something along the lines of:

//! Temporary substitute for std::unwrap_ref. This will be replaced by
//! std::unwrap_ref once all our compilers support it. The names in this
//! file do not conform to the coding style to ease the eventual transition
//! to std::unwrap_ref.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -785,7 +786,7 @@ void CMemoryUsageTest::testDynamicSizeAlwaysZero() {
CPPUNIT_ASSERT_EQUAL(haveStructPodCompilerSupport, test);
test = core::memory_detail::SDynamicSizeAlwaysZero<std::pair<int, int>>::value();
CPPUNIT_ASSERT_EQUAL(true, test);
test = boost::is_pod<SFoo>::value;
test = std::is_pod<SFoo>::value;
Copy link
Contributor

Choose a reason for hiding this comment

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

You can now delete the comment on lines 762 to 770.

Also I imagine now haveStructPodCompilerSupport should always be true, so you could get rid of that too and replace with literal trues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@droberts195
Copy link
Contributor

Due to the limitations of the std::ref implementation, boost::ref cannot be removed where it is used in Input and Output filters (e.g. lib/core/CCompressOStream.cc and lib/core/CStateDecompressor.cc)

Sorry, I just realised this is why most of the extra places my first grep found still exist. But I think most of the headers that are still included that the second one found don't need to be included now.

@valeriy42
Copy link
Contributor Author

recheck

Copy link
Contributor

@droberts195 droberts195 left a comment

Choose a reason for hiding this comment

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

LGTM

@valeriy42 valeriy42 merged commit 9c96e37 into elastic:master Jun 24, 2019
@valeriy42 valeriy42 deleted the boost-to-std branch June 24, 2019 05:40
@valeriy42 valeriy42 restored the boost-to-std branch June 24, 2019 05:40
valeriy42 added a commit to valeriy42/ml-cpp that referenced this pull request Jun 24, 2019
* Replace boost functions with std where possible

* UnwrapRef: Safeguard fix and documentation

* CMemoryUsageTest.cc: get rid of outdated comment and is_pod checks

* Remove superfluous includes

* Fix windows compiler bug

* Fix formatting

* Added missing comment
valeriy42 added a commit that referenced this pull request Jun 24, 2019
Following boost functions have been raplaced with their std equivalents: boost::array, boost::begin, boost::end, boost::is_pod, boost::ref, boost::cref, boost::reference_wrapper, boost::make_unique, boost::algorithm::is_sorted, boost::bind.

Due to the limitations of the std::ref implementation, boost::ref cannot be removed where it is used in Input and Output filters (e.g. lib/core/CCompressOStream.cc and lib/core/CStateDecompressor.cc).
@valeriy42 valeriy42 deleted the boost-to-std branch June 28, 2019 06:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ML] Migrate boost functionality to std alternatives
2 participants