Remove Boost from API #480
Conversation
Codecov Report
@@ Coverage Diff @@
## master #480 +/- ##
==========================================
+ Coverage 59.94% 59.98% +0.03%
==========================================
Files 382 383 +1
Lines 40677 40758 +81
Branches 6807 6810 +3
==========================================
+ Hits 24385 24448 +63
- Misses 14317 14333 +16
- Partials 1975 1977 +2
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Thanks for submitting this PR -- I'm certainly supportive of minimizing dependencies for downstream code. It's probably worth adding something regarding this particular issue in CONTRIBUTING.md, e.g. that Boost includes should not be made in the public interface, except if wrapped in #ifndef CANTERA_API_NO_BOOST.
If I'm reading this correctly, the only capability that you lose if you define CANTERA_API_NO_BOOST is that you can't instantiate any new specializations of anyValue.as<T>() etc., is that right? If so, that should be fine. This is a new feature added since the last release in any case, so backwards compatibility is not a concern.
| #include <string> | ||
| #include <vector> | ||
| #include <memory> |
There was a problem hiding this comment.
I think this should come in implicitly via global.h -> ct_def.h.
There was a problem hiding this comment.
I'll let those chain-include then.
include/cantera/base/AnyMap.inl.h
Outdated
| { | ||
|
|
||
| // Definitions for templated functions which require the full declaration of | ||
| // class AnyValue/AnyMap. |
There was a problem hiding this comment.
This is no longer just those functions that require the full class declaration -- it's all of the definitions.
include/cantera/base/AnyMap.inl.h
Outdated
|
|
||
| #include <string> | ||
| #include <vector> | ||
| #include <unordered_map> |
There was a problem hiding this comment.
A lot of these includes are redundant with what's already coming in via AnyMap.h
src/base/xml.cpp
Outdated
| #include <sstream> | ||
| #include <fstream> | ||
|
|
||
| namespace ba = boost::algorithm; |
There was a problem hiding this comment.
I'm wondering whether it would be better to just have wrappers in stringUtils.h/cpp for the 3 functions that are being used over and over again (iequals, to_lower_copy, and trim_copy), rather than the extra include and namespace declaration in so many files. Thoughts?
There was a problem hiding this comment.
I started down this exact path with a to_lower_copy wrapper but abandoned it as scope creep when I started hitting more "ba" functions. I do think it would be better to try to corral most of the boost usage into a couple files so I'll try another pass since it may be just those three functions.
src/base/AnyMap.cpp
Outdated
| @@ -5,6 +5,10 @@ | |||
|
|
|||
| #include "cantera/base/AnyMap.h" | |||
|
|
|||
| #ifdef CANTERA_API_NO_BOOST | |||
| #include "cantera/base/AnyMap.inl.h" | |||
| #endif | |||
There was a problem hiding this comment.
This should be unnecessary, right? CANTERA_API_NO_BOOST should never be defined when compiling Cantera, so the .inl file should have been included via AnyMap.h.
There was a problem hiding this comment.
Yep. This was an artifact from when I thought I need to do something at compile time. Removed!
That is correct. |
- Limits propagation of boost header and namespace.
- boost now only included in AnyMap.inl.h (within include/cantera)
Removes Boost headers from the API. This removes the requirement for clients to have these headers when building against Cantera.
I tried to make this as backwards compatible as possible (minor/patch). The only real non-backwards compatible pieces are the removal of the (directly unused) boost header/namespace from stringUtils.h and possibly the change to the size/layout of AnyValue). Otherwise, it is opt-in with CANTERA_API_NO_BOOST.
It seems that the generic capability of AnyMap/AnyValue isn't really used within Cantera or on its interface so it could probably just be removed if you were interested in breaking changes to the API. It is technically used in a couple places but they are all calls like as that could be replaced by asString.