XML/mod validation

[daniel.santos]

hailstone, I noticed you're doing work on code to validate the XML and give you a list of programs rather than the 1st one, an oft-requested feature.  I wanted to first make you aware of the new Checksums class in the network branch.  This is designed to say more than "your files differ" in a network game.  Instead, it will be able to tell you which files differ.  I think this will only help when messing with mods that are in the development stages and haven't been officially released, but since you were touching that code, I thought I would mention it to you.

Next, the way you went about it is very copy-pastey and bloated.  Actual bloat of generated code isn't that big of a deal in this part of the code because it's only run when a game starts, so that part doesn't bother me.  However, I would prefer the copy & paste part to be encapsulated in some type of macro.  I'm even not opposed to constructs that some would call butt-ugly like this:

Code: [Select]

#define DONT_DIE(x) try{ (x); } catch (runtime_error e) {Logger::getErrorLog().addXmlError ( path, e.what() ); loadOk = false;}

This allows you to call potentially exception-throwing functions like this:

Code: [Select]

DONT_DIE(ProducibleType::load(parametersNode, dir, techTree, factionType));

Really though, don't use the name DONT_DIE, that part's a bad example. =)  Encapsulating it in a macro also gives us the ability to compile it out by changing the macro with some pre-processor directive.  For blocks where you can't make them a one-liner, you can have a start and end macro something like this:

Code: [Select]

#define DONT_DIE(x) try{ (x); } catch (runtime_error e) {Logger::getErrorLog().addXmlError ( path, e.what() ); loadOk = false;}
#define DONT_DIE_START try{
#define DONT_DIE_END } catch (runtime_error e) {Logger::getErrorLog().addXmlError ( path, e.what() ); loadOk = false;}

Were this something that would easily be broken into separate function calls (for each potentially throw-able operation) I would prefer the Inversion Of Control pattern, but I think that macro-izing what you've already done is a descent solution to this problem.

oops! I just noticed that sometimes you do need to bail, so maybe add a parameter that specifies rather or not to bail on failure?

Code: [Select]

#define DONT_DIE(a, b) try{ (a); } catch (runtime_error e) {Logger::getErrorLog().addXmlError ( path, e.what() ); loadOk = false; if(b) return false;}
#define DONT_DIE_START try{
#define DONT_DIE_END(a) } catch (runtime_error e) {Logger::getErrorLog().addXmlError ( path, e.what() ); loadOk = false; if(a) return false;}

So other than the copy & paste, it looks like good work

[hailstone]

I'm not sure what you mean by validation. I suggested some programs to use and did some error checking for TinyXML but I wouldn't call that xml validation.

Can you say what files you're looking at?

Nice work with the Checksum. Could the different files be transferred over to make them the same (temporarily) or would this be too much of a security risk?

[daniel.santos]

Oh, I'm sorry! I wasn't very clear!

Like trunk/source/glest_game/types/unit_type.cpp:107 from, in bool UnitType::load(int id, const string &dir, const TechTree *techTree, const FactionType *factionType, Checksum &checksum);

[daniel.santos]

Nice work with the Checksum. Could the different files be transferred over to make them the same (temporarily) or would this be too much of a security risk?

Yea, I'm not too sure about that part yet, although it would be ideal.  I guess as long as it gets a lot of peer-review it would be OK.  And also as long as it prompts the user and confirms that they want to allow it.  In the long run, having a central server where mods are formally published to would be the best solution.

EDIT: I should add that the code is already in the game to transfer files too!  See NetworkMessageFileHeader and NetworkMessageFileFragment trunk/source/glest_game/network/network_message.h:333.  The code that manages this currently is hard-coded to only allow them to the savedgames directory.

EDIT2: Ahh, the other code is here: trunk/source/glest_game/network/server_interface.cpp:221 and here trunk/source/glest_game/network/client_interface.cpp:34.

[hailstone]

Like trunk/source/glest_game/types/unit_type.cpp:107 from, in bool UnitType::load(int id, const string &dir, const TechTree *techTree, const FactionType *factionType, Checksum &checksum);

Ah ok, that's not my work though. I only modified xml_parser.cpp/h.

I was thinking about using the installer to download mods but It's probably better discussed at https://forum.megaglest.org/index.php?topic=4472.0

[silnarm]

That was me

It is indeed very copy-pastey, but it's just for loading XML anyway...  The macros are possibly a good idea, but also possibly not. I don't know that I personally want too many 'fancy' macros like this in the source.  The trouble is it could get out of hand too easily... What you've suggested here so far is not so bad I guess... but if more modifications and additional macros are needed... well.. I think I'd prefer the odd bit of copy-paste source bloat, in this case its contained to xml loading functions that you rarely look at anyway...

[daniel.santos]

Yea, like I said, may would consider my approach "butt-ugly", and I agree with you about what can happen when macros get out of hand.  Have you ever worked/been forced to work with MFC? But, gladly, what you wrote appears to get the job done and people have been asking for that for some time now

[silnarm]

Yeah, MFC was an abomination.

I'm not totally against this kind of thing though, I considered something similar for the Lua error handling, but macros in that case was out of the question, and I just 'fudged it'... it's also fairly copy-pastey, and could use a generic checkArguments() function, something for another day

[daniel.santos]

... something for another day

Yea, let's just try to not let those "other days" get too far away.  We'll probably balance each other out well because I tend to over-perfect and thus, screw the whole pooch by taking on too much!  But I had similar macros when I was doing JNI/C++ stuff (Java Native Interface, where you implement functions of a Java class in C or C++).  I had this whole framework to catch C++ exceptions and translate them into Java exceptions that was fairly seamless, except for the "BEGIN_STUFF" AND "END_STUFF" macros at the start and end of each JNI function's C implementation! =)  However, I understand that the GNU folks have done an amazing job at this integration via gjc, too bad I missed out on that when I Java work was my bread & butter.

That was one cool thing about stuff like this, with anonymous local inner classes (that's really what they're called) I could do this w/out the ugly stuff using IoC and define the anonymous local inner class right inside my function body just like it were a snippet of code in an if() block (except it had two sets of braces):

Code: [Select]

    myIocObject.run(new SomeAbstractClass() {
        void somePureVirtualFunction() {
            // do stuff that throws exceptions
        }
    });

    myIocObject.run(new SomeAbstractClass() {
        void somePureVirtualFunction() {
            // do more that throws exceptions
        }
    });

Thus, the entire class definition is never given a name (i.e., anonymous), is instantiated only in the body of this function and has access to all local variables in current function's scope at the time it's instantiated (i.e., the "local").  It's an "inner" class because it belongs to the parent class, of which it can also access members of if it needs to.

But when trying to do something cleanly starts getting ugly, it defeats the purpose, so I follow you.

[silnarm]

Yeah, I learnt Java when it was shiny new, I think anonymous inner classes were introduced in 1.1? I saw them, but stopped programming in Java soon there after, .NET lets you do similar things with delegates, you get anonymous methods though, not classes   Such features can be used like you have there for nice elegant solutions to some problems, unfortunately we get nothing of the like in C++.  The pre-processor allows you arguably the ability to do more, but it does of course come with dangers  

I'm also wary of scaring off potential contributors with a mess of custom macros.

[daniel.santos]

I don't know STL as well as I should.  I wonder if Boost's or TR1's function objects can provide us the solution we want.  I'll look at this more closely later.