On 10/06/2015 11:15 PM, Mike
Miller wrote:
I like the
overall concept, makes the code more direct and easier to
understand and maintain. My nit to pick is that the calling
convention
is not very clear:
std::string octave_value::string_value (const
std::string& msg);
Looking at the class definition of octave_value, it's not at
all clear
what the difference is between the two string_value methods,
or why one
would be preferred over the other.
Yeah, it is a little strange to be passing in an error message
just in case. I would not want to do that for very many
functions. But it seems OK to me for value extraction since it
is necessary and common to handle incorrect argument types and
this allows a very concise way to extract a series of values
while providing pretty good diagnostics in case of failure:
std::string url = "" ("URL must be a
string");
std::string output_file = args(1).string_value ("OUTPUT must
be a string");
etc.
If adding these extra member functions seems like clutter, then
the next best option seems to me to be a regular function like
this:
std::string url = "" (args(0), "URL must be a
string");
But is that really better than just defining the extractor as a
member function?
Should this
error-state-setting variant be called something else? Or
should this be the way more functions are going to tend to go?
For this
specific case, I see that string_value(true) is never used in
Octave,
should this new method replace that one completely?
I think the bool argument is just historical baggage left over
from a time when I thought that value type conversions would be
more useful and common than they turned out to be. We could
work to eliminate it.
Unfortunately, since the current extractor has a bool argument,
we have to define both
string_value (const char *)
string_value (const std::string&)
instead of just the one that accepts std::string. Otherwise,
calling
string_value ("foobar");
will call string_value (bool) instead of using the std::string
constructor to convert the character constant to a std::string
object. So I'm not sure what we can do here to make this change
both soon and smoothly. If we don't care at all about backward
compatibility, we can just drop the bool version and define
string_value (const std::string& = std::string ())
If the bool argument of the current string_value extractor
doesn't actually do anything, then I'm not sure it matters
much. We wouldn't suddenly (and silently) be making code behave
differently, we'd just be requiring people to remove irrelevant
arguments from function calls. Disruptive, yes, but fairly
simple to change.
Does any code that we know of derive from octave_base_value,
overload the string_value extractor, and make the bool argument
do something?