[Scummvm-devel] Decompiler: Documentation feedback

[Michael Madsen]

> -----Original Message-----
> From: Max Horn [mailto:max at quendi.de]
> Sent: Monday, August 09, 2010 6:55 PM
> To: Michael Madsen
> Cc: 'ScummVM devel'
> Subject: Re: [Scummvm-devel] Decompiler: Documentation feedback
> 
> >
> > The main advantage over not going straight to PDF is that if you use
> > graphics, going to DVI forces you to use EPS (vector graphics), while
> > pdflatex allows bitmaps, which (being raster images) don't scale well.
> > Of course, seeing that there aren't any graphics in there at this point,
> > I guess it doesn't hurt to change it, so I can do that later today.
> I am not sure I get your point here. Are you saying you prefer to go
> via DVI, because it prevents you from using raster/pixel graphics? That
> seems like a rather weak reason (you could simply not rasterized
> graphics with pdflatex, too ;).

Yes, well, habit plays a fairly large part in this as well. ;)

Like I'll said, I'll change it later today.

> 
> [...]
> 
> >> * 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.
> >
> > If having small bits of raw, inline code isn't an appropriate use of
> \verb, what *is* an appropriate use of \verb? \texttt will certainly
> get the same visual layout, but now you have to think about escaping as
> well, and I've been taught that's why \verb is a better choice for
> these things.
> 
> If you were quoting code involving special characters (e.g. HTML tags
> like "<table>") , and not just identifiers, I would understand the use
> of \verb (although I still would consider a custom, \verb like macro
> better -- just like I consider using hyperref's \url, which is a \verb-
> like macro, better for URls than \verb). But for identifiers only,
> essentially the only things you get from using \verb is (a) it switches
> to the typewriter font, and (b) it disables hyphenation.
> 
> Most importantly, using a macro also serves to separate the semantics
> from the presentation/style. If we wanted to switch to printing code in
> an orange calligraphic font, we'd then have to modify only one place,
> namely that macro. Right now, we'd have to track down all uses of \verb,
> and decide whether it is used for quoting code, or something else.
> Moreover, \verb has many subtle and dangerous effects. E.g. it is a so-
> called fragile command, meaning you can't use it in section headers and
> certain other places.

Fair enough. It'll take a while to get them  all changed, but I'll get it done sometime during the week.

> 
> >
> >> 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).
> >
> > I don't know enough about v3 to say one way or the other about that.
> If it can't be autodetected from the file itself,
> 
> Those differences affect only certain opcodes and in general are very
> hard to detect, or impossible in some cases; an automatic "guesser"
> would be possible, but if it guesses wrong, the user should be able to
> override it.
> 
> 
> > but can be restricted to specific platforms, then the platform switch
> (see below) would be usable there; otherwise you would either need
> another subclass of Engine.
> 
> Which would be identical to the first subclass, except for a handful of
> opcodes :).

Ah, but you have to remember, there's no requirement that an Engine subclass can't use a class used by another engine. Nothing prevents you from adding a parameter to your disassembler constructor which is used to determine how to handle those handful of opcodes.

In fact, you could just subclass the "extra" Engine from the first Engine and only override the getDisassembler method to pass in another value for that constructor parameter, and check that in your disassembler...

> 
> Not that it would be too difficult, at least for the disassembler i am
> not using the SimpleDisassembler, which has the nice advantage that I
> can put the opcode handling in a separate method, which is called from
> a loop in doDisassemble(); I can then overload that function in a
> subclass, handle only the "special" opcodes there, and otherwise invoke
> the parent class' implementation.
 
...or use a subclass overriding only the part that's different, yes. It depends on what sort of differences you're dealing with.

> I guess it would be possible to modify SimpleDisassembler a bit to
> achieve a similar thing there, if one wanted to.
> 

You can even do it with SimpleDisassembler; you can just use OPCODE_BASE to provide a custom implementation for those particular opcodes, or put that stuff in a method of its own and override it in a subclass. OPCODE_BASE is described in 3.5.5 Advanced opcode handling.

> 
> > You could add an engine-specific switch, but (and I admit I may be
> > missing something here...) Boost.ProgramOptions does not appear to
> > provide a good way of doing so, unless you're just going to send a
> > string (or an array of strings) into the engine/disassembler/code
> > generator and let it deal with them however it wants.
> 
> Hm, that's of course not that pretty either. But then having a "talkie"
> command line switch in a decompiler seems not so pretty to me, either -
> - no offense intended, I just mean that both feel quite hackish to me.
> Now I read that you also are thinking about adding a platform option.

The "difference" is that talkieness and platforms can be applied in a more general fashion; it's not tied to a single engine. You can also add demos to that list; I don't know of any cases where that property would make a difference, but I can easily see that it could.

> 
> Hrm. How about this alternative suggestion then: Every engine class can
> (optionally) provide a list of "variants" or "subengines". I.e. add a
> new method which returns a (possibly empty) list of strings. Such as
> "pc", "amiga", "talkie", "amiga-talkie", "pc-demo". The "-l" option
> would print next to each engine the list of subengines (each could have
> a custom description text). The user could *optionally* specify a
> subengine via a new command line flag.
> 
> This way, you can combine both the platform selector and the talkie bit,
> as well as cover the SCUMM cases I have in mind. Of course, this starts
> to get ugly if you have lots of independent flags that can occur in
> multiple combinations: If e.g. a game comes in
>  | {pc, amiga, mac} x {talkie, non-talkie} x {demo, non-demo} | = 12
> variants, it would be nicer to have three separate flags, instead of 12
> combined "subengine" names (also for the user, who would dislike having
> to remember if it was "demo-pc-talkie" or "talkie-pc-demo" or any of
> the other 4 possible orderings).

That seems like it's going to require some form of pattern matching to find the "best" available subengine. I can see the point in letting engines tell you what switches that can affect them, but I'm not completely sure it's a good idea to actually use subclasses for every combination; I suspect that the changes required for each one are relatively isolated, so perhaps it's better to do what is generally done in ScummVM itself and just use if statements where necessary.

> For SCUMM, this hypothetical issue does not exist. How about Kyra, I
> wonder?

Kyra2 has a single magic function that takes an extra parameter from the stack in the talkie version, while Kyra1 reads one of the IFF chunks (ORDR) slightly differently for two platforms (FM-Towns and PC98). I haven't checked the magic functions in Kyra1, so there might also be something there.

I have no idea about the other games using Kyra; each game uses a different set of magic functions, but that's pretty much all I know about them.

> >>
> >> * 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...
> >
> > The only thing outside your engine that depends on the parameter type
> > is the CFG output, and that's only to provide a special case for
> > kString (to escape the string for dot output). They're pretty much
> > there for completeness, in case you want to use that metadata somewhere
> > in your engine-specific code.
> 
> OK. Would be nice if the docs mentioned just that (either the LaTeX
> docs, or the doxygen comment of the class).

I'll work that in.

Michael