[Scummvm-devel] Decompiler: Documentation feedback

[Michael Madsen]

> -----Original Message-----
> From: Max Horn [mailto:max at quendi.de]
> Sent: Wednesday, August 11, 2010 4:54 PM
> To: Michael Madsen
> Cc: ScummVM devel
> Subject: Re: [Scummvm-devel] Decompiler: Documentation feedback
> 
> Hi there,
> 
> some more feedback:
> 
> * In 3.4, after consulting the source code, it seems to me that
> _addressBase should be in SimpleDisassembler, not Disassembler, if at
> all.

At the very least, it should be available for SimpleDisassembler so you're not restricted to loading your scripts at address 0x0. (Alternatively, you'd have to supply the initial address as a parameter to START_OPCODES - that would also work.)

Whether or not it should be available for Disassembler as well - I suppose you could handle that on your own instead, if you have such a need - but I don't know if that's necessarily better or worse.

> * In 3.4, do we really need to document _disassemblyDone at all? It
> seems like an internal implementation detail to me (In fact, I'd
> consider making it 'private'). Instead, I'd change the documentation
> for disassemble() to more explicitly mention that it will call
> doDisassemble(), but caches the result, so if called again it uses the
> cached data. Hence doDisassemble() is called at most once.

Yes, that probably makes more sense. I'll change it.

> 
> * In 3.5.4, in the example it says
> 	OPCODE_MD(0x14, "add", kBinaryOp, -1, "", "}");
> Why the "}" and not a "+" ? If it is on purpose, there should be an
> explanation.

That's a typo.

> 
> * In 4.2, it says literally "stack effect >= 0". I'd rewrite this as
> either "non-negative stack effect", or (less preferable) "stack effect
> $\geq 0$".

Sure, let's go with "non-negative".

> 
> * In 5.5, you explain that 0xC0 is handled in a special way. It should
> be made explicit that this simply means that processInst is invoked
> (resp. that the instruction will be treated as described under "Other
> types", regardless of its type).

Agreed.

> * Related to this, the processInst doxygen comment is rather fuzzy. It
> should at least give a brief overview of where it is called, and when /
> for which instructions (kind of a 2-3 line summary of section 5.5).  It
> would

...it would what? :) But yes, that comment could probably do with some fleshing out.

Michael