Examples: Use modern C++ #81
Conversation
|
I'm currently disinclined to apply this PR though I do appreciate your efforts Murray. |
murraycu
force-pushed
the
develop-murrayc-examples-modern-cpp
branch
6 times, most recently
from
f4675cd to
5b82ca3
Compare
|
I would love to see Boost.Graph get a modernization, I would actually vote for C++14 at this point. |
|
@Belcourt why are (were) you disinclined? You rejected the PR but gave no reason for the submitter to allow for more improvement. |
|
I'm helping out with the PR backlog. Looks like you have a large code change. I would like to merge this in around v1.69. The modernization is greatly appreciated, but making some fixes available and then having this change at a later version. There is a very long mailing thread where it looks like the latest when C++03 support will be thought about is 2020, but I'm comfortable telling people now that they should be using a newer system. This can affect other PRs, so lets hold off on merging everything now. This is to let you know and help me prioritize PR's. |
|
Should we actually make this a branch for devel-cpp17? |
@anadon That's encouraging. Thanks. I don't quite understand what you'd like to do and when. Please feel free to request changes or updates from me that would help you to get this merged into the real code base. |
|
@murraycu Can you please rebase this PR on upstream's devel branch? |
murraycu
force-pushed
the
develop-murrayc-examples-modern-cpp
branch
from
6bd1dda to
dfcd947
Compare
|
OK. I've rebased on master and force pushed to the develop-murrayc-examples-modern-cpp branch in my fork. I can't seem to remember how I built this at the time, inside modular-boost, so it might not actually build now. |
|
@murraycu Looks like there are some CI failures. Could you please take a look at those? |
|
2 failures are in the code changes, 2 are from external parameter parsing. Shouldn't be that bad to fix. |
|
At the moment I can't even build the regular develop branch: |
Use emplace_back() and emplace_front() instead of push_back() and push_front().
Instead of array + length. And use initializer lists.
This is not necessary in C++11 and it is not a common style.
Because it makes the code less clear.
This could be improved with "structured bindings" if that gets into C++17, but that alone would still not allow a range-based for loop. http://eli.thegreenplace.net/2016/returning-multiple-values-from-functions-in-c#structured-bindings-a-new-hope-for-c-17
So we can use auto.
These are meant as documentation, not code, so just leave them commented out. This lets the build succeed with -Werror.
murraycu
force-pushed
the
develop-murrayc-examples-modern-cpp
branch
from
dfcd947 to
a22ddcc
Compare
murraycu
force-pushed
the
develop-murrayc-examples-modern-cpp
branch
from
a22ddcc to
99b1a6e
Compare
|
The remaining build errors in CI are in tests/isomorphism.cpp, and seem to be unrelated to my changes. |
|
I've been out a while, but I think an external dependency changes and all
of graph broke. I'd lets things settle while graph gets fixed back up.
…On Thu, Nov 29, 2018 at 2:52 PM Murray Cumming ***@***.***> wrote:
The remaining build errors in CI are in tests/isomorphism.cpp, and seem to
be unrelated to my changes.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#81 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AC-CBbJV6rHErOodtiziho7-JN2SmtcUks5u0DsEgaJpZM4KnIyE>
.
|
| // Inspired by: | ||
| // https://cplusplusmusings.wordpress.com/2013/04/14/range-based-for-loops-and-pairs-of-iterators/ | ||
| template <typename T_Pair> | ||
| class range_pair |
There was a problem hiding this comment.
I don't think this class is a good idea, sorry. For starters, it's not how the library is actually used. Secondly, there must be something equivalent in Boost.Range already, presumably iterator_range.
Thirdly, and more importantly, the best thing to do would be to add begin(boost::pair)/end(boost::pair) functions to graph_traits.hpp or some other common file so that boost::pair is automatically treated as a range. If that causes problems, an alternative would be to return iterator_range instead of boost::pair.
Now, that should be done separately from this PR so that this remains entirely about updating examples.
Finally -- the reason I am suddenly interested in this, is we have now effectively dropped support for pre-C++11, so we can actually consider merging!
OK. Rebasing this is a bit awkwad after the reformatting in 2019, so I'll make a series of smaller pull requests instead. |
These commits change the code in examples/ to use modern C++, specifically C++11. For instance, to use auto to avoid the verbosity of the traits and typedefs.
Even if we don't want to require C++11 for the API itself, maybe it's OK to use it for the example code, to more clearly demonstrate the API. I would understand if you don't want this.
The last commit ("Add and use make_range_pair") is even more debatable: It uses a range_pair utility class to allow use of range-based for loops, but maybe this should actually be a change to the API, or maybe it's something that will be better one day with C++ ranges in C++20.