cc expression methods; passed by value tempoaries seem avoidable

[blauzahn]

I see no reason why the methods:

Code

ExpressionNode::ParseNodeType(wxString token)

ExpressionNode::IsBinaryOperator(wxString first, wxString second)

Expression::AddToInfixExpression(wxString token)

get their parameters passed by value. To me it looks unnecessary inefficient if used often enough.
Don't we have a whole tree with lots of those?

I recommend to change these to const wxString&.


PS:

It we were in c++11-land, AddToInfixExpression may actually get its parameter passed by value, if
m_InfixExpression is empty most of the time and

Code

m_InfixExpression.push_back(token);

were changed to something like:

Code

m_InfixExpression.emplace_back(std::move(token));

... but that's a different story;-)

[oBFusCATed]

wxString in wx2.8 has COW semantics, so these are pretty cheap.
But you can always provide a patch, with improvements.

[blauzahn]

patch: #003558

wxString in wx2.8 has COW semantics, so these are pretty cheap.

Hasn't that changed with  wx-2.9? And even then, COW is more work than no work.

[oBFusCATed]

If you've profiled or benchmarked the code then we can draw conclusions about slow and fast parts.
Everything else is just guessing and people are notoriously known to fail to guess correctly

[killerbot]

the patched version is definitely much better style, lead by example ;-)

[oBFusCATed]

We're not talking about style, but performance...

[killerbot]

doesn't matter, is better code anyway, don't depend on whatever wxStrings might optimize or not optimize. Const ref wherever possible.

[blauzahn]

@obfuscated: I agree with you that measuring is vastly superior to guessing.
And I consider it absolutely critical to make sure that no regressions creep in.
Be it functional or in performance. Thanks for taking that serious. I want my
beloved cb to work after an update, safely and responsive.

This quite tiny change of signature to const& in this case is rather to
increase clarity, showing the intent (query, probably no sink), beware
(myself) of accidentially changing the parameter within the routine ---
but without sacrificing any performance!

No, I haven't gprofed it yet. Do you have some measurements for this case?
Instead I tried for this tiny change to focus on the correctness and even
punched in a good dozen of Unittest++ Checks into a separate file. Just
to make sure I do not break anything (Testing is for whimps only, right?).
Unfortunately, tests for AddToInfixExpression are still to be done because
it is not testable as simple. And I do not consider #ifdef CC_PARSER_TEST
being a sensible automatic unittest strategy.

More effective-C++ ways to be more efficient-C++ (pun intended) can be found
rather elsewhere E.g.:

Code

ExpressionNode::ExpressionNode()
{
    Initialize(wxEmptyString);
}

void ExpressionNode::Initialize(wxString token)
{
    m_UnaryOperator = false;
    m_Token = token;
    m_Type = ParseNodeType(m_Token);
    m_Priority = GetNodeTypePriority(m_Type);
}

All data-member are assigned a value twice when the default-ctor is called
and that is done in an inner loop further down. Did you notice that
the order of the assigments is different from the data-member sequence.
Not that this matters here. But that's how errors creep in.

I wanted to start at the smallest building blocks. Having confidence
in them before I work my way further up.

The real dragons lie are, where the complexity is high (have you fed
pmmcabe with CC?) and local reasoning is hard due to friendship,
inheritance, protected data-member and naked pointers. As for performance:
Did you ever try a sizeof(Token). How many do we have?

I hope, that future replies can be a lot shorter;-)

[oBFusCATed]

You're still guessing! Profile first than make changes, but do not use gprof it will probably be a waste of time.
Use perf, cachegrind, oprofile, vtune or zoom (last two are proprietary)

For testing there is a separate project cctest, I don't know if it is automated or not.
But if you can provide some unittest++ test it won't hurt, probably you can integrate it in the cctest project...