REFAC(client,server): boost calls replaced with STL ones #6731
Conversation
g-virus-dev
force-pushed
the
no-boost
branch
from
d5833ca to
a225c58
Compare
There was a problem hiding this comment.
Generally speaking, I am quite happy with this PR. I think using nullopt_t instead of {} might make the code a bit clearer in some cases but it's not a major thing. So if someone else (especially @g-virus-dev) disagrees, this is nothing I'll insist on.
Mostly the changes need to be split into different commits where each commit contains all code changes related to a single logical refactoring. Once this is all done, the code needs to be formatted to conform to our formatting rules but this has to be done with clang-format v10 for now so I could also do this in the end.
Finally, with Boost Accumulators I am not sure whether completely removing Boost is the way to go. I usually prefer reusing existing libs instead of having to reimplement their functionality (especially Boost as that's pretty much omnipresent in the C++ ecosystem).
.gitignore
Outdated
| @@ -17,7 +17,7 @@ indent.sh | |||
| *.vcxproj | |||
| *.vcxproj.* | |||
| *~ | |||
| *.pro.user | |||
| *.user | |||
There was a problem hiding this comment.
What is the relation of this change to removing the Boost dependency?
| @@ -45,7 +45,7 @@ BonjourServiceResolver::BonjourServiceResolver(QObject *p) : QObject(p) { | |||
| } | |||
|
|
|||
| BonjourServiceResolver::~BonjourServiceResolver() { | |||
| foreach(ResolveRecord *rr, qmResolvers) | |||
| for (ResolveRecord *rr : qmResolvers) | |||
There was a problem hiding this comment.
While these changes are very much welcome, I would prefer having them in a separate commit that is disentangled from the Boost removal.
| @@ -111,10 +111,6 @@ def main(): | |||
|
|
|||
| wrapperContent = create_disclaimerComment() | |||
|
|
|||
| # Include boost-bind as we'll need it later | |||
| wrapperContent += "\n#include <boost/bind/bind.hpp>\n\n" | |||
There was a problem hiding this comment.
I guess we should include <function> instead.
src/CMakeLists.txt
Outdated
| @@ -180,7 +180,7 @@ target_link_libraries(shared | |||
| OpenSSL::SSL | |||
| Qt6::Core | |||
| Qt6::Network | |||
| Qt6::Xml | |||
| Qt6::Xml | |||
There was a problem hiding this comment.
Unrelated formatting change - please remove as tabs are used above it as well.
src/ServerResolverRecord.h
Outdated
| const QString& hostname() const; | ||
| quint16 port() const; | ||
| qint64 priority() const; | ||
| const QList< HostAddress >& addresses() const; |
There was a problem hiding this comment.
These changes have nothing to do with removal of Boost and should therefore be in a separate commit
| #ifndef Q_MOC_RUN | ||
| # include <boost/scoped_ptr.hpp> | ||
| # include <boost/shared_array.hpp> | ||
| #endif |
src/murmur/Messages.cpp
Outdated
| for (ChanACL* a : c->qlACL) | ||
| delete a; |
There was a problem hiding this comment.
Please leave explicit braces in the code.
src/murmur/Messages.cpp
Outdated
| const MumbleProto::ACL_ChanGroup &group = msg.groups(i); | ||
| g = new Group(c, u8(group.name())); | ||
| Group* g = new Group(c, u8(group.name())); |
src/murmur/RPC.cpp
Outdated
| foreach (c, chans) { | ||
| foreach (User *p, c->qlUsers) | ||
| for (const auto& c : chans) { | ||
| for (User *p : c->qlUsers) |
There was a problem hiding this comment.
While touching this code, please add explicit braces around loop body
src/murmur/UnixMurmur.cpp
Outdated
| @@ -62,8 +62,10 @@ void LimitTest::testLimits(QCoreApplication &a) { | |||
| if ((count & 1023) == 0) | |||
| qWarning("%d descriptors...", count); | |||
| } | |||
| foreach (QFile *qf, ql) | |||
|
|
|||
| for (QFile *qf : ql) | |||
This commit replaces boost classes with its STL alternatives
This commit replaces boost calls that can be simply replaced with STL ones
g-virus-dev
force-pushed
the
no-boost
branch
from
a225c58 to
38212d9
Compare
g-virus-dev
changed the title
There was a problem hiding this comment.
It also looks like you introduced a lot of unrelated whitespace changes. Please use clang-format in version 10 (!) on these files. This should be done for each commit separately, not once in the end. Otherwise, we end up with single commits that are not passing the CI.
Furthermore, you added a merge commit by accident.
| @@ -45,7 +45,7 @@ list(APPEND CMAKE_MODULE_PATH | |||
| ) | |||
|
|
|||
| if(NOT DEFINED CMAKE_CXX_STANDARD) | |||
| set(CMAKE_CXX_STANDARD 17) | |||
| set(CMAKE_CXX_STANDARD 20) | |||
There was a problem hiding this comment.
I'd say please use a separate, dedicated commit just to raise the C++ version
In this dedicated commit, also change this doc: https://github.com/mumble-voip/mumble/blob/master/docs/dev/build-instructions/README.md to include cpp20 instead of 17
|
And your changes are not compiling quite yet. See CI logs for details. |
This commit replaces boost calls with STL ones