[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: plot templates and options lists for set, plot etc.
From: |
John W. Eaton |
Subject: |
Re: plot templates and options lists for set, plot etc. |
Date: |
Mon, 4 Jan 2010 16:10:00 -0500 |
On 4-Jan-2010, Thorsten Meyer wrote:
| Thorsten Meyer wrote:
| > Thanks for your feedback (and your offline reminder :-) ).
| >
| > Included you will find an updated patch. See comments below.
| >
| > John W. Eaton wrote:
| >> On 16-Sep-2009, Thorsten Meyer wrote:
| >>
| >> | Thorsten Meyer wrote:
| >> | > Here is a patch implementing struct and cell array arguments for the
set function.
| >> | >
| >> | > I have impplemented the matlab behaviour (as I understand it ...). See
the tests
| >> | > added to the patch for the details.
| >> | >
| >> | > Could somebody please review the patch (what I do in the sources is
still a lot
| >> | > of trial and error):
| >> | > - Have I got the types of the various variables right?
| >>
| >> | > - Is it ok to add doxygen style comments to the new methods? (the
| >> | > doxygen docs
| >>
| >> I don't use Doxygen, so I'm unlikely to remember to add the markup in
| >> the comments I write. If we decide to do this, then I think we should
| >> try to do it on a more global scale.
| > I removed the Doxygen markup for now.
| >
| >> | > - What about the coding style?
| >> | > - Are the added methods for the graphics_object ok (also where I
placed the
| >> | > definitions)?
| >>
| >> I try to keep the order of the declarations in the .h file the same as
| >> the order in the .cc file. If you've done that, then it is probably
| >> OK.
| > I fixed the order to be consistent in .h, .cc and the documentation string.
| >
| >> | +//! Set properties given in two cell arrays containing names and values.
| >> | +void
| >> | +graphics_object::set (const Array<std::string> names,
| >> | + const Cell values, octave_idx_type row)
| >>
| >> I think this should be
| >>
| >> graphics_object::set (const Array<std::string>& names,
| >> const Cell& values, octave_idx_type row)
| > done.
| >
| >> | + if (! (names.numel () == values.columns ()))
| >>
| >> Why use
| >>
| >> ! (x == y)
| >>
| >> instead of
| >>
| >> x != y
| >>
| >> ?
| > I changed it.
| >
| >> | +void
| >> | +graphics_object::set (const Octave_map m)
| >>
| >> This should be
| >>
| >> void
| >> graphics_object::set (const Octave_map& m)
| > Done
| >
| >> | +//! Set a property to a value or to its (factory) default value.
| >> | +void graphics_object::set_value_or_default (caseless_str& name,
| >> | + octave_value& val)
| >>
| >> I think this should be
| >>
| >> void
| >> graphics_object::set_value_or_default (const caseless_str& name,
| >> const octave_value& val)
| >>
| >> unless the function needs to modify NAME and VAL. It doesn't appear
| >> need to.
| >>
| >> | + if (val.is_string ())
| >> | + {
| >> | + caseless_str tval = val.string_value ();
| >> | +
| >> | + if (tval.compare ("default"))
| >> | + val = get_default (name);
| >> | + else if (tval.compare ("factory"))
| >> | + val = get_factory_default (name);
| >> | + }
| >> | +
| >> | + if (error_state)
| >> | + return;
| >> | +
| >> | + rep->set (name, val);
| >> | +}
| >>
| >> With the non-const VAL, if VAL is a string, then it will also be
| >> changed in the caller. Is that really what you want? I think it
| >> would be better to define a local temporary value inside the
| >> function.
| > I changed the method arguments as you suggested and blew up the conditional
a
| > bit to avoid copying val.
| >
|
| here is a kind reminder for the patch above and an updated patch. Can it be
| applied as it is?
I checked it in.
Thanks,
jwe