Skip to main content
added 5 characters in body
Source Link

I know, the example is incomplete and canshould be discussed controversialcontroversially. Nevertheless, it is good to know.

I know, the example is incomplete and can be discussed controversial. Nevertheless, it is good to know.

I know, the example is incomplete and should be discussed controversially. Nevertheless, it is good to know.

added 50 characters in body
Source Link

But as you said, RecvBuffer is external... :-/

But as you said, RecvBuffer is external... :-/

Source Link

Even the question is so what out dated here some things I see on your code

Copy Assignments

You should remove the = on lines like

MyClass foo = {};

and rather do

MyClass foo {};

The difference is, that with the = First an rvalue will be instantiated, after that the move assignment operator will be called. This makes a difference in performance when used with big-data classes or if the class does not provide copy or move assignments.

But mind expressions like that:

auto myNumber {42};

Which are quite fine, as long as you're using c++17. On earlier versions you'll get a initializer-list instead of an integer. Anyhow, writing something like:

int myNumber{42};

is less ambiguous and more typesafe and in case of

auto groupBuffer = RecvBuffer {buffer.current, buffer.current + groupSize};

the following may increase performance, does not need RecvBuffer to by copiable or movable and does rely on compiler optimizations less.

RecvBuffer groupBuffer {buffer.current, buffer.current + groupSize};

Passed by Value

In general, you should avoid passing by value, except build-in types (int, bool, etc). Better pass by reference. That comes to my mind when I saw lines like:

bool MessageManager::handlePackageData(uint32_t lastAck, uint32_t seqNumber, RecvBuffer buffer)

Buffer is the key that triggered my when reading the parameter name RecvBuffer. You possibly do a lot of copying there.

Getter, Setter and Encapsulation

You seam to grant access to some internal data i.e. on

if(before == groupBuffer.current)...

think about RecBuffer::getCurrent() const. If current is a build-in type, it may simply get the internal current data for you. Then, you are a bit saver, when simply reading form current and avoid unintentional assignments. Think about:

while(groupBuffer.getCurrent() < groupBuffer.end) {
            try {
                // make your intend clear! Just reading -> const auto
                const auto before = groupBuffer.getCurrent();
                auto ret = false;

                {
                    static const auto scopeSource = "network::mm::[messageHandler]"_src;
                    dlg::SourceGuard sourceGuard(scopeSource);
                    ret = messageHandler_(groupSeq, groupBuffer);
                }

                // avoid infinite loop, message handler MUST advance buffer
                // turn your condition. Even if `before` is not const
                // a missing = will never lead to an assignment
                if(groupBuffer.getCurrent() == before) {
                    dlg_error("invalid messages handle did not advance buffer");
                    return false;
                }

                if(!ret) {
                    dlg_warn("invalid pkg: handler return false");
                    return false;
                }
            } catch(const MsgBufInvalid& err) {
                dlg_warn("invalid pkg: messageHandler threw: {}", err.what());
                return false;
            }
        }

Exceptions

You are mixing up exceptions with return codes. Maybe, you need to. I do not know your requirements on the interface of your class. But because you allready use exceptions in your project, think about to rethrow exceptions, after caught and after calling dlg_warn. When I was in a similar scenario of code I implemented my own exception class and at places where you are returning false, I throw my excpetion, constructed with meaningful data. The caller of my function needs to wrap a try/catch and gets a meaningful error-state if something fails.

... something like:

        while(groupBuffer.current < groupBuffer.end) {
                auto before = groupBuffer.current;
                auto ret = false;

                {
                    static const auto scopeSource = "network::mm::[messageHandler]"_src;
                    dlg::SourceGuard sourceGuard(scopeSource);
                    ret = messageHandler_(groupSeq, groupBuffer);
                }

                // avoid infinite loop, message handler MUST advance buffer
                if(before == groupBuffer.current) {
                    //dlg_error("invalid messages handle did not advance buffer");
                    throw MessageManagerExcpetion(Type::NotAdvanced);
                }

                if(!ret) {
                    //dlg_warn("invalid pkg: handler return false");
                    throw MessageManagerExcpetion(Type::HandlerError);
                }
        }

...
try{
 inst.handlePackageData(...)
}
catch(MessageManagerExcpetion& mexp){
  dlg_warn(mexp.what());
}
catch(...){
  dlg_warn("Something unexpected happend here!");
}

I know, the example is incomplete and can be discussed controversial. Nevertheless, it is good to know.