[Gs-code-review] Implementing Visual Trace service - improved

[Igor V. Melichev]

Raph, Russell, Dan,

Thank you for your comments.
Replying all messages in one batch.
I'll delay sending the new improved patch (because it exceeds the mail list
message size limitation)
until I get a response from Raph about this.

> http://www.ghostscript.com/mailman/listinfo/gs-code-review
> From:	Russell Lang [gsview at ghostgum.com.au]
> Sent:	22 ??????  2002 ?. 11:02
> To:	code review; Igor V. Melichev
> Subject:	Re: [Gs-code-review] Implementing Visual Trace service - improved

> Don't document it in API.htm, only in the source files and
> Lib.htm.  If reimplemented in a platform independent way,
> we don't want to have to maintain backward compatibility.

Perhaps set_visual_tracer is a platform independent call.

> I'm pleased that the tracing code is only accessed from
> dwmain.c dwmainc.c for a DEBUG build, but the conditionals
> are incomplete. graphical_tracer_init is surrounded by a
> conditional, but graphical_tracer_close is not. Please do a
> test build without DEBUG.

Without your DWTRACE It builds and work fine, but I'll add #if for symmetry.

> dwtrace.obj should only be linked in for DEBUG.
> I suggest something like the following in winint.mak:
> !if $(DEBUG)
> DWTRACE=$(GLOBJ)dwtrace.obj
> !else
> DWTRACE=$(GLOBJ)dwtrace.obj
> !endif
> OBJC=$(GLOBJ)dwmainc.obj $(GLOBJ)dwdllc.obj
> $(GLOBJ)gscdefs.obj $(GLOBJ)gp_wgetv
> .obj \
> $(GLOBJ)dwimg.obj $(GLOBJ)dwreg.obj $(DWTRACE)

Done, but :
1. Not sure what we win with it. Modern linkers skip uncalled code without
our help.
   Windows linkers do.
2. Probably you mean :

!if $(DEBUG)
DWTRACE=$(GLOBJ)dwtrace.obj
!else
DWTRACE=
!endif

> dwtrace.c:
> - Contains "fixme" note, for something that would take a
> lot of work to fix.

Sure.

> - Contains a little 16-bit code.  I've been removing 16-bit
> code from Windows files when it is clearly not needed.

Removed, rather your statement isn't correct.
The code does not contain 16bit code.
But it skips some code if compiled to 16 bit or to win32s.

> The current implementation is a kludge.
> I hope Igor agrees :)

I agree with Raph's comment about this.

> My preference is for a different implementation which uses
> the display device unmodified in the client.  This would
> require that ghostscript to contain code to render to a
> bitmap, which requires duplication of part of the graphics
> library. Although this is a cleaner solution that is
> platform independent, it is a lot more effort which may not
> be warranted for debugging.

Feel free to improve it, rather I'm not sure about the purpose.
We have plans to replace entire gswin32 and gswin32c with a MFC-based code.

> From:	gs-code-review-admin at ghostscript.com on behalf of Raph Levien
[raph at casper.ghostscript.com]
> Sent:	22 ??????  2002 ?. 11:20
> To:	Russell Lang
> Cc:	code review; Igor V. Melichev
> Subject:	Re: [Gs-code-review] Implementing Visual Trace service - improved

> > - ReturnIfNoWindow
> [...]
> I strongly favor removing the macro entirely. It comes across as a
> "trick", which is just a little bit more concise, but definitely
> harder to follow.

I've removed it.
IMHO the code became much longer and overloaded with minor details.

> I also dislike the CheckRET macro, now introduced into gxfill.c,

Here is an improved version :

#define CheckRET(a) do { int temporary_variable_for_this_macro;
\
                         if ((temporary_variable_for_this_macro = a) < 0)
\
                             return temporary_variable_for_this_macro;
\
                       } while(0);

I'd like to have it globally defined.
It's a very widely used pattern, being similar to 'throw' (from C++).

I would make more tricks with __LINE__, but not sure about portability.


> > My preference is for a different implementation which uses
> > the display device unmodified in the client.  This would
> > require that ghostscript to contain code to render to a
> > bitmap, which requires duplication of part of the graphics
> > library. Although this is a cleaner solution that is
> > platform independent, it is a lot more effort which may not
> > be warranted for debugging.
>
> Such a thing should be added to the "Projects.htm" file. We also
> discussed doing a separate X implementation, but that's probably not
> much less code than drawing into a bitmap. One hurdle that the
> "display" implementation would have to address is getting a font for
> rendering the text. This sounds like a fair amount of extra work.

Not sure about *this* thing (the purpose isn't clear), but will do about X.

> There are numerous // comments in the code. This is not portable
> to all targets. We should probably add a regression check for //
> comments, as they seem to be a relatively common occurrence even
> after several prior iterations.

Removed.
Perhaps libpng and some device drivers contain a lot of // comments.

> The name set_debug_flafs appears to be a typo.

Thank you. Fixed.

> I noticed some deeper changes to gxfill.c in addition to merely adding
> the visual trace (most notably, disabling the scan line fill
> algorithm). If left in the code, this will likely cause problems with
> PCL clients of the graphics library, as well as the PDF 1.4
> implementation. Ideally, these changes would be in a different
> commit. However, as I imagine you'll be working on gxfill.c, if you
> plan to fix these problems soon after the commit, they are acceptable
> for now.

This is my mistake. This change doesn't relate to vdtrace,
now removing it from the patch.

> The use of M as a variable name is not consistent with the rest
> of Ghostscript. Traditionally, variables are lowercase, while
> uppercase represents the use of a macro, or a constant. Some people
> may also be bothered by the fact that m and M are both used in the
> same scope.

Replaced with 'pm'.

> From: gs-code-review-admin at ghostscript.com
[mailto:gs-code-review-admin at ghostscript.com]On Behalf Of Dan Coby
> Sent: Friday, February 22, 2002 12:02 PM
> To: Raph Levien; Russell Lang
> Cc: code review; Igor V. Melichev
> Subject: RE: [Gs-code-review] Implementing Visual Trace service - improved

> Why is the CheckRET macro in this code.  Igor suggested it back in
> June of last year.  No one (except Igor) liked the idea.  I was
> definitely of the opinion that no one wanted it in the Ghostscript
> sources.  Given that response, I am very surprised to hear that it
> is being used anyway.

My strong opinion is to discontinue excessive conservatism in Ghostscript.
CheckRET is an attempt to emulate C++ operator 'throw' (rather in a very
simplified way). I am open about improving this macro. Here is another
fragment of my code from zfapi.c, line 903 :


        if (code == e_limitcheck) {
            /* The server provides an outline instead the raster. */
            gs_imager_state *pis = (gs_imager_state *)pgs->show_gstate;
            gs_point pt;
            CheckRET(gs_currentpoint(pgs->show_gstate, &pt));
            CheckRET(outline_char(i_ctx_p, I, import_shift_v, penum_s,
pgs->show_gstate->path, !pbfont->PaintType));
            CheckRET(gs_imager_setflat(pis, gs_char_flatness(pis, 1.0)));
            if (pbfont->PaintType) {
                CheckRET(gs_stroke(pgs->show_gstate));
            } else
                CheckRET(gs_fill(pgs->show_gstate));
            CheckRET(gs_moveto(pgs->show_gstate, pt.x, pt.y));
        }

My criteria are :

1. A bigger part of algorithm can fit into window, so it is better
observable.
2. Can set breakpoint on any line or call.
3. It is widely used pattern.

Currently there are occurances in fapiufst.c,
gxfill.c (not committed yet except HINTER branch),
gxshade1.c, zfapi.c - total 98 occurances.

Igor.