#1041 Add proper version comparation to compiler's XML parser

[Miguel Gimenez]

This patch adds the following verbs to the XML parser:

• version_greater
• version_greater_equal
• version_equal
• version_not_equal
• version_less_equal
• version_less

comparing strings in the form major[.minor[.patch[.tweak]]] but not limited to four parts.

See this forum thread

[Teodor Petrov]

Thank you for the contribution. It is a good start but I think it needs a bit more work.

I have 3 complaints about this patch:

1. I don't like the if else chain of calls to GetAttribute. I think it is too expensive to iterate the attributes all these times. I think you should redo the loop from wxXmlNode::GetAttribute:

    wxXmlAttribute *attr = GetAttributes();
    while (attr)
    {
        if (attr->GetName() == attrName)
        {
            *value = attr->GetValue();
            return true;
        }
        attr = attr->GetNext();
    }

This will get you to a single iteration. You can get even fancier/faster if you first check for attrName[0]=='v' and then get to the slower/fuller checks in an if else fashion.

1. I don't like the calls to wxSplit. Hidden calls to new/delete like these are the #1 contributor to slowness in C::B. I think it is possible to write this comparison doing 0 allocations, and iterating the strings 1 or 2 times. And I think it shouldn't be that hard. I think you need to use the wx equivalent of https://en.cppreference.com/w/cpp/string/byte/strtol
2. If any of the two versions fail to parse as version an error must be reported in the logs. I suppose it is fine you just use the debug log.

[Miguel Gimenez]

I made the requested changes. As a side effect you can now write more than one test, and the result will be the AND of all.

Calls to CmpVersion() are repeated, but the idea is "don't execute if not needed".

[Morten MacFly]

I'm taking this one, too...

[Teodor Petrov]

@mortenmacfly I've done the initial review, so I can finish it.

@Miguel Gimenez

Currently I don't like this part:

    // wxStrtol() is not documented and not recommended, see https://wiki.wxwidgets.org/Undocumented_Parts_Of_WxWidgets
    // Stupid char** in strtol...
    char *pf = const_cast <char *> (static_cast <const char *> (First.c_str()));
    char *ps = const_cast <char *> (static_cast <const char *> (Second.c_str()));

I don't like those c_str() calls and all the casts.
This is from the docs:

wxCStrData wxString::c_str  (       )   const

Returns a lightweight intermediate class which is in turn implicitly convertible to both const char* and to const wchar_t*.

Given this ambiguity it is mostly better to use wc_str(), mb_str() or utf8_str() instead.

Given there is no strtol equivalent in wx I would do the number parsing/conversion manually. Given how much you already have adding a value accumulator and *=10 to it would be fine and probably simpler. I'd probably do it with a separate function like int extractNumber(int *result, const wxString &s, int searchStart); or just inline in this one. Whatever is less code and looks simpler.

[Miguel Gimenez]

I have created a GetNextValue(const wxString& s, size_t* Index, size_t Length) method and removed xx_str() calls completely.

[Teodor Petrov]

The last patch fails to apply:

> patch -p1 --binary < /tmp/mozilla_obfuscated0/compiler3.patch
patching file include/compiler.h
patching file sdk/compiler.cpp
Hunk #1 FAILED at 1234.
1 out of 2 hunks FAILED -- saving rejects to file sdk/compiler.cpp.rej

1. I don't like that GetNextValue is a member function. It is better to put it as global static function in compiler.cpp
2. _T is something from the past
3. Don't use F. Use wxString::Format directly. F has problems, but I don't remember the details.
4. GetNextValue can return true/false if there is a failure to parse.
5. CmpVersion could also be made global function.
6. Ideally we have to setup some tests, but we currently doesn't have a way. :(

[ollydbg]

Don't use F. Use wxString::Format directly. F has problems, but I don't remember the details.

F() is not thread safe, because it use some static/shared wxstring if I remember correctly.

[Miguel Gimenez]

I copied the call to DebugLog with F and_T from other part of the code, they are used elsewhere

I don't know how compiler3.patch got so flawed, it even lacks most part of the new code. Here is the last version.

[Teodor Petrov]

@Miguel Gimenez I've applied your last patch and made some modifications. You can see them here https://github.com/obfuscated/codeblocks_sf/tree/experiments/compiler_version_check I've broke something or you need to do any more changes please post patches against this branch.

What I've changed:

1. Capitalization of local variables. I'd prefer if you use lowercase first letter. No matter what is written in the style guide, the style of cb is mostly lowercase first letter for variables.
2. Use wx_str() when formatting strings, I think not doing it would lead to a crash, but I've not investigated much.
3. Translate error message and prefix them.
4. Fail the comparison if one of the versions fails to parse. I don't know why you've made it differently before
5. Handle .. I've added a test for it and it failed.
6. Added temporary tests.
7. Don't use pointers in loops, generally this is slow if the function is non-inlined, because the compiler cannot optimize away the accesses.

[Miguel Gimenez]

Starting with wxWidgets 3.0 you can use wxString as argument for wxPrintf, from the docs:

// This is the recommended way.
wxPrintf("You can do just this: %s", s);
wxPrintf("And this (but it is redundant): %s", s.c_str());
wxPrintf("And this (not using Unicode): %s", s.mb_str());
wxPrintf("And this (always Unicode): %s", s.wc_str());

[Teodor Petrov]

@Miguel Gimenez Do you have any comments on my changes to your patches?

[Miguel Gimenez]

Only the one above about wxPrintf

[Teodor Petrov]

In svn, thanks. Let me know if I've messed something up.