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.
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.]
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.
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.
One more remark: tabs should be used for indentation.
And, of course — thanks!
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.
+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.
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.
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.
