[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: imwrite parameter framework patch (with full gif support)
From: |
John Swensen |
Subject: |
Re: imwrite parameter framework patch (with full gif support) |
Date: |
Fri, 1 Oct 2010 06:56:54 -0400 |
On Oct 1, 2010, at 2:49 AM, John W. Eaton wrote:
> On 1-Oct-2010, John Swensen wrote:
>
> | You may want to make some mods. I have been trying to look at the
> | changes you make to my patches to see if they are stylistic things
> | that I can start doing to save core maintainer's time on future
> | patches. Let me know if you see any glaring bugs/holes. I plan on
> | starting to work my way through each of the format types and
> | submitting another patch each time I complete an image format.
>
> There are some style guidelines in the Octave manual in the current
> sources.
>
> Here are some additional specific comments.
>
> +2010-09-39 John P. Swensen <address@hidden>
> +
> + * image/imwrite.m: Added function for default image options.
>
> Please write the name of the function, for example:
>
> * image/imwrite.m (default_options_for_format): New subfunction.
> Use it to set default formats.
>
> +function options = default_options_for_format (fmt,img)
>
> Please put a space after commas in argument lists.
>
> + options = {};
> + switch (fmt)
> + case 'bmp'
> + # Do nothing; no default options, but not a warning.
>
> Please use ## for indented comments, for the benefit of Emacs Octave
> mode indenting.
>
> + case 'gif'
>
> Please use "" to quote strings unless there is some good reason not
> to.
>
> + warning (["imwrite - No default options exist for format: " fmt]);
>
> Please take advantage of the format possibilities of the warning and
> error functions when appropriate, and don't captitalize error
> messages. For example:
>
> warning ("imwrite: no default options exist for the %s format", fmt);
>
> + end
>
> Please always use specific "end" tokens. In this case, "endswitch"
> instead of end.
>
> +2010-09-30 John P. Swensen <address@hidden>
> +
> + * DLD-FUNCTIONS/__magick_read__.cc (write_image): Added
> + framework for all supported image types.
> + (<format>_settings): Implemented functions for each supported
> + image format with skeleton code.
> + (gif_settings): Completely implemented all GIF settings.
> +
>
> Please include entries for all new functions. Say what was added or
> changed, not simply that something was added or changed.
>
> +std::string ptolower (std::string str)
> +{
> + std::transform(str.begin(), str.end(), str.begin(), tolower);
> + return str;
> +}
>
> If this function is only used locally, it should be static. If you
> intend to make a copy of the argument, I would prefer that it be
> explicit. Also, please follow the whitespace rules as described in
> the style guide in the manual (and generally, the GNU coding
> standards). For example:
>
> static std::string
> ptolower (const std::string& s)
> {
> std::string r = s;
> std::transform (r.begin (), r.end (), r.begin (), tolower);
> return r;
> }
>
> However, if you need caseless strings for property names or values,
> maybe it would be better to use the caseless string class defined in
> graphics.h.in? Maybe we should split that class out of graphics.h and
> put it in its own file? It seems likely that it would be useful in
> more places than just graphics.h or graphics.cc, but it doesn't make
> sense to require you to include all of graphics.h just to get caseless
> character strings.
>
> + /*
> + * BackgroundColor
> + * Comment
> + * DelayTime
> + * DisposalMethod
> + * Location
> + * LoopCount
> + * ScreenSize
> + * TransparentColor
> + * WriteMode
> + */
>
> Please use C++ comments.
>
> + // NOTE: for future coders, the order in which properties are processed is
> important.
>
> Please limit the length of lines, including comments, to less than 80
> characters if possible.
>
> + bool isAppendMode = false;
>
> Please don't use CamelCase. Instead, write is_append_mode.
>
> + try {
>
> Please follow the GNU coding style and put braces on lines by themselves.
>
> + else
> + {
> + error ("imwrite: invalid type for 'gif' property '%s' (expects a
> string)", "writemode");
> + }
>
> If there is only one statement following an else, there is generally
> no need to use braces. Also, in this error message, there is no need
> to use a format specifier when you have a literal string corresponding
> to it. Instead, write
>
> else
> error ("imwrite: invalid type for 'gif' property 'writemode'
> (expects a string)");
>
> + if (!((dimensions (0) == 2 && dimensions (1) == 1) || (dimensions
> (0) == 1 && dimensions (1) == 2)))
>
> Please split long conditionals before the || and && operators, like
> this:
>
> if (!((dimensions (0) == 2 && dimensions (1) == 1)
> || (dimensions (0) == 1 && dimensions (1) == 2)))
>
> In the comment
>
> + // Actually, ML makes it sound like they use values of the
> colormap
> + // for the background color. Maybe this is so they don't
> actually introduce
> + // another color to the colormap. This may be problematic as I
> don't think
> ...
>
> it is OK to say Matlab (I don't know what ML is) but it doesn't make
> sense to say "they" when referring to Matlab. Maybe you mean to say
> that "The Matlab docs seem to imply that colormap values are used for
> the background color. Maybe this is so that it is not necessary to
> introduce another color into the colormap. ..."?
>
> Would you please make these changes and resubmit the patch?
>
> Thanks,
>
> jwe
Oh my!. Sorry for all the problems. I didn't realize I was making you fix
quite that much stuff in my previous patches. I guess most of my previous
patches were much smaller. I will get everything fixed and re-submit.
John