[Spice-devel] [PATCH v3] DisplayChannel: document exclude_region() functions

[Jonathon Jongsma]

On Wed, 2017-04-12 at 08:30 -0400, Frediano Ziglio wrote:
> > 
> > On 04/12/2017 12:33 PM, Frediano Ziglio wrote:
> > > > 
> > > > This is a particularly opaque part of the code for managing
> > > > pending
> > > > Drawable operations. This patch adds documentation atempting to
> > > > explain
> > > > these functions.
> > > > ---
> > > > 
> > > > So, I sent an 'amendment' to my documentation patch several
> > > > weeks ago (see
> > > > email with subject "Amend the previous commit to change the
> > > > "XXX"
> > > > comments"),
> > > > but it never got reviewed. Here's the original documentation
> > > > patch with
> > > > the
> > > > amended patch merged together. Maybe it will be easier to
> > > > review this
> > > > time?
> > > > 
> > > 
> > > As far as I know these comments are fine and doubts are
> > > explicitly
> > > marked.
> > > Looking at the overall maybe could be helpful if instead of "I"
> > > and "me"
> > > you use your nick. Will avoid having to look at the history of
> > > the file.
> > 
> > Or replace with a general FIXME: line.
> > For example: replacing
> >      + * NOTE: I still don't have a great conceptual understanding
> > of
> >          this function's + intended use.
> > with
> >      + * FIXME: what is the intended use of this function.
> > 
> > 
> > Uri.
> > 
> 
> The problem of the FIXME is that usually they refer to wrong stuff in
> the code.

I'm not sure that it needs to mean that. It could mean that we need to
fix/improve the comment. But nevertheless, if you don't want to use it
that way, that's fine. Another option is "TODO:". (fwiw: vim syntax
highlights both FIXME and TODO in the same way.)

Jonathon