Trunk discussion / complains

Guys, I’ve run vcmi through cppcheck again, there are some things that bother me.
server/CGameHandler.cpp

if ( (s1->tempOwner != player && s1->getStackCount(p1) < s1->getStackCount(p1) )
			|| (s2->tempOwner != player && s2->getStackCount(p2) < s2->getStackCount(p2) ) )
if (s->mainEffectAnim > -1 || (s->id >= 66 || s->id <= 69) || s->id == SpellID::CLONE) //allow summon elementals

I think these are ctrl+c - ctrl+v problems, the second one definitely requires && instead of ||.

launcher/modManager/cmodlistmodel_moc.cpp

case ModFields::STATUS_ENABLED:
		{
			return (mod.getModStatus() & (ModStatus::ENABLED | ModStatus::INSTALLED))
			     < (mod.getModStatus() & (ModStatus::ENABLED | ModStatus::INSTALLED));
		}
		case ModFields::STATUS_UPDATE:
		{
			return (mod.getModStatus() & (ModStatus::UPDATEABLE | ModStatus::INSTALLED))
			     < (mod.getModStatus() & (ModStatus::UPDATEABLE | ModStatus::INSTALLED));
		}

Also not very clear to me.

AI/VCAI/VCAI.cpp

if (newMovement == oldMovement) //means our heroes didn't move or didn't re-assign their goals
			{
				logAi->warnStream() << "Our heroes don't move anymore, exhaustive decomposition failed";
			}
				break;
			if (safeCopy.empty())
				break; //all heroes exhausted their locked goals
			else

Wouldn’t it break after the first break and nothing down this break would be never executed?

Also there are some things like

The struct 'HireHero' defines member variable with name 'player' also defined in its parent struct 'CPackForServer'.

I assume such redefining is useless, tell me if I’m wrong.

Attached report.txt (which is report.xml) for those who’re interested.
report.txt (26.6 KB)

Thanks. I’ll commit these in a several days unless someone beats me to this.

Code should be OK but it may be a good idea to rename them to something else. This can be caused by code like this:

struct A
{
    int ID;
}

struct B : public struct A
{
    std::string ID;
}

This is valid code that will create struct with two members with same name. When working with B you will access string version and when working with A - int version. Internally these are two completely separate fields so code is OK but it may be a good idea to make different names for them.

All but this one should be fixed:

Not sure what’s correct logic here. Can somebody take a look on it? CGameHandler.cpp, line 2300

Fixed this one.

I have observed another issue — if modSettings.json is not present, VCMI does not start. modSettings.json is created as an empty file, and VCMI subsequently fails to start, because it cannot find its core resources.
[This can be worked around by opening Launcher and enabling core graphics mod.]

That change in modSettings format turned out to be more problematic than I expected it to :frowning:
Should be fixed now.

It’s something strange with ENABLE_PCH option. In CMakeLists.txt file:

if(ENABLE_PCH)
	include(cotire)
endif()

But in other files cotire function is used without that check.

If I enable PCH I cann’t build the game:

Rev: 3729

It all worked fine until I rebuilt entire solution. Now keep getting errors:

Error	2	error LNK2001: unresolved external symbol "unsigned long __cdecl _exception_code(void)" (?_exception_code@@YAKXZ)	E:\Programowanie\VCMI\vcmi\lib\Connection.obj
Error	11	error LNK2001: unresolved external symbol "unsigned long __cdecl _exception_code(void)" (?_exception_code@@YAKXZ)	E:\Programowanie\VCMI\vcmi\server\CVCMIServer.obj
Error	3	error LNK1120: 1 unresolved externals	E:\Programowanie\VCMI\VCMI_lib.dll	1
Error	12	error LNK1120: 1 unresolved externals	E:\Programowanie\VCMI\VCMI_server.exe

They are so meaningless I have no clue where to look at.

The issue was on my side - Nvidia Nsight extension is very unreliable. Reinstalling Visual helped.
Also, I’m going to install MVS 2013 in soon.

I’m glad to see it solved. The error was completely puzzling to me, I had no idea how to approach it.

Then two warnings:

  • Qt. They do not provide official build for MSVC 2013 and building it is quite troublesome. Let me know when you upgrade, I’ll upload my binaries.
  • you’ll need a post-1.55 build of Boost (I just grabbed sources from Git and compiled them). The 1.55 release… works except for a few parts that VCMI depends on.
    When downloading from git make sure to checkout the “develop” branch, not the “master”. **

Now PCH should be enabled only if it’s enabled:) I think there were 2 CMakeLists (BattleAI don’t know) where I forgot to add the check.

Instead of the prefix header generated from cotire the StdInc.h will be used. This should eventually fix the compilation error. As a drawback PCH will be used for all projects even for StupidAI with only one source file. We can remove cotire there, but I think the speed improvement will be small. Full rebuild is still 1:40 minutes.

@AVS:

I have a few notes on your latest commits to SVN.

  • We use camel case for naming variables, functions and classes/types. The underscore shouldn’t be used.
  • You have a lot of lambda functions in CSpellHandler.cpp which introduce some more detailed logic and shouldn’t be mixed with the more abstract outer methods. Ideally you should use lambda functions, which have a few lines of code and the same level of abstraction, in conjunction with algorithms like find_if, remove_if, count_if,… I would use private methods instead of lambda functions.
  • What’s get_nomber? I think you mean get_number
  • I would move CSpell inlines to .cpp file. C++ is fast anyway.
  • It seems to be that your code formatter removed unnecessary tabs and spaces. Perhaps you can turn this off or reformat whole code base in one commit and developers should use the same formatting options. Otherwise SVN diff will generate a lot of noise.

Nevertheless you did a good job, thanks!

One more remark: tabs should be used for indentation.

And, of course — thanks! :slight_smile:

I guess it depends on the individual taste.

Actually I really like using lambdas not only for STL algorithms but as a way of defining local functions, that 1) allows to avoid repeating code within a function 2) does not leak function implementation details outside. I’d say the boundary between “is fine as a lambda” and “should be private member function” is whether the code piece in mind can be actually reused outside its original function. :slight_smile:

+1
Other reasons:

  • code in headers generally increases build-times and scope of needed rebuilds after a change
  • compilers know better whether to inline functions and either totally ignore “inline” keyword or treat as a hardly significant suggestion. When we really know better than compiler that function should be inlined and when that matters, we use STRONG_INLINE macro.
  • Naming fixed, inlines converted to normal methods will commit this soon.
  • Code formatter: sorry, forgot to disable this in new IDE installation.
  • A lot of lambdas need a bit more time to fix. Will try to convert some of them to functions reusable in other handlers and move to maybe JsonUtils.

As for lambdas - another issue is that they’re largely unnecessary. For example this one:

    auto readFlag = ](const JsonNode& flagsNode, const std::string& name)
    {
        if (flagsNode.getType() != JsonNode::DATA_STRUCT)
        {
            logGlobal->errorStream() << "Flags node shall be object";
            return false;
        }

        const JsonNode& flag = flagsNode[name];

        if (flag.isNull())
        {
            return false;
        }
        else if (flag.getType() == JsonNode::DATA_BOOL)
        {
            return flag.Bool();
        }
        else
        {
            logGlobal->errorStream() << "Flag shall be boolean: "<<name;
            return false;
        }
    };

check for struct: not necessary - check for correct type will be done during validation
check for boolean - same as above
check for null: not necessary - default value for bool is false anyway

So this lambda can be replaced with one line:
flag = node"flag"].Bool();

One of ideas behind schemas support was specifically to avoid endless checks like these in code.

Agreed. Done some simplification.

Can’t compile Launcher on Windows.

4>imageviewer.obj : error LNK2001: unresolved external symbol "public: virtual struct QMetaObject const * __thiscall ImageViewer::metaObject(void)const " (?metaObject@ImageViewer@@UBEPBUQMetaObject@@XZ)
4>imageviewer.obj : error LNK2001: unresolved external symbol "public: virtual void * __thiscall ImageViewer::qt_metacast(char const *)" (?qt_metacast@ImageViewer@@UAEPAXPBD@Z)
4>imageviewer.obj : error LNK2001: unresolved external symbol "public: virtual int __thiscall ImageViewer::qt_metacall(enum QMetaObject::Call,int,void * *)" (?qt_metacall@ImageViewer@@UAEHW4Call@QMetaObject@@HPAPAX@Z)

Linker seems unable to see some Qt-generated methods.
Try running it again. After files are added, the first compilation generates the Qt moc files, and they are visible in subsequent compilations.

This is because I forgot to add _moc suffix to new file. Will fix soon.

Thanks, that worked.