[Scummvm-devel] Decompiler: Documentation feedback

[Max Horn]

Hi again,

Here is my feedback, based on trying to add a "SCUMM v5" engine to the decompiler. This also sparked some more general feedback, affecting your code, not just the docs. I am not yet finished with reading your docs, nor with that v5 disassembler (haven't even started the decompiler part), but since this email already was long enough... consider it as "part 1" ;).


LaTeX style remarks:
* Consider using pdflatex instead of latex+dvips+ps2pdf.
* It would be nice to have \usepackage{hyperref} in there; that way references become hyperlinks, and the resulting pdf (if using pdflatex) gets a TOC. Very handy. You may have to put that after variorref, though.
* What do you need the "url" package for? I took it out and things still compiled fine (note that \url is also provided by hyperref).
* Likewise for \usepackage{bbold} 
* Please don't use:
  \verb+ _functions+
Instead, define your own macro to mark things as code, e.g. 
  \newcommand{\code}[1]{\texttt{#1}}
If you also want the argument to be not hyphenated, then that can also be achieved cleanly, w/o abusing \verb.


Section 2.1 "Adding a new engine"
=================================
Some more info here would be nice. You just say that one has to invoke the ENGINE macro. You don't mention (although I admit it is fairly obvious) that one also has to #include ones own header. I added:
  #include "scummv5/engine.h"

Next, you say one needs to implement certain methods, but you don't say in what (sub)class one has to do that. Taken both together, you may want to suggest as first step that one should pick an engine name ("scummv5" in my name), and then create a subdir for that, including an engine.h file which contains a subclass of class Engine.
It would be nice if the getDisassembler & getCodeGenerator entries linked to the sections where the relevant classes are described.


Sidenote: you currently name the subclasses like v6::Engine (which is a subclass of ::Engine), personally, I'd prefer ScummV6Engine or so, even if it resides in its own namespace. Some old compilers, and more importantly, developers (of any age ;) are confused by classes with identical names in different namespaces. "Hey, you have a bug in class Engine" -- "Which one do you mean" ?

Back to the docs. It might be a good idea to create a simple (fake) decompiler as a running example. Of course that's even more work, but would be helpful. As a somewhat less expensive solution, you could at least point out what the corresponding files / classes for the Kyra / ScummV6 engine are, so that one at least knows where to quickly find some real world examples. (Again, this is not *that* difficult, but if one argues that one can find out about things by reading the code, then one can quickly argue that documentation and comments are pointless in general... ;)

At the end of this section, you refer to the "Kyra2" code generator. This is the "name" of this engine, but the class is named Kyra::Engine. I found that confusing...

Section 2.2 "Game information"
==============================
First line: "it's -> "it is". This and things like "there's", "you'll", etc. are colloquial and should be avoided in a formal text like that.


I would rephrase the first sentence a bit:
"For some engines, it may not be enough to know the engine; some instructions may differ in behavior between different games or variants of a game, for example between talkie or non-talkie versions, or between versions for different platforms."

Non-docs-remark: This is something that affects many engines in one way or another, in particular the SCUMM engine. E.g. for v3: You have to specify whether the data format is blocked or unblocked (so I *hope* it will be possible to auto-detect that), whether it is Indy3-256 or Zak256 (FM-TOWNS version). 
Your code currently has a "talkie" switch, but for platform variants, you suggest using multiple engine subclasses. Why? Why not require the talkie variant to use a subclass, too? Or why not allow specifying the platform variant with a switch? The current situation seems a bit arbitrary. Personally, I think allowing to specify an engine "variant" or "subengine" or so with a generic switch might be easier (and would clutter the list of engines less) than requiring lots of subclasses.
However, we probably should wait with change to this until we determine which engines would be affected and how... :)



Some random remarks
===================
* I noticed that Scumm::v6::Disassembler::doDisassemble generates no error when an unknown blockName is encountered (that should be resolved). In fact, there seems to be little error checking in general, nor a "framework" for doing that. Of course I could just invoke error there, but for unknown instructions an exception is used... So there is no clear "error handling strategy".

* Is there any particular reason why the Parameter type distinguishes between 8, 16 and 32 bit values? Naively, it would seem kInt and kUInt are sufficient for all needs. Mind you, I am not necessarily suggesting to abolish those, I am just wondering whether it is important for a disassembler to use those correctly -- i.e. does anything outside of my own engine depend on these distinctions in any way? This is not clear to me from the docs, and in the code I only see getUnsigned & getSigned methods returning an int, so...



Bye,
Max