Re: [lmi] Help modifying 'rate_table.cpp'

[Vadim Zeitlin]

On Thu, 15 Dec 2016 06:02:29 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2016-12-14 18:16, Vadim Zeitlin wrote:
...
GC> > [...] I'd actually prefer to do this on reading the table,
GC> > not when writing it. For me logically adjusting the number of decimals 
used
GC> > is part of validation and so should be done as early as possible, i.e. in
GC> > validate().
GC> 
GC> Thanks, I did it that way. Pushed 82b89079ffcdd3702b4c09b3c5455e4a7a3650e0:
GC>     Detect and repair decimal-precision inconsistencies in rate tables

 This looks nice and simple to me, thanks!

GC> > GC> Or
GC> > GC> maybe the right place for this change is
GC> > GC>     soa_v3_format::table_impl::write_as_text()
GC> > GC> so that it would be excluded from
GC> > GC>     soa_v3_format::table_impl::write_as_binary()
GC> > GC> but that breaks the implicit assumption that those two functions
GC> > GC> share the same implementation.
GC> > 
GC> >  I think it's nice to reuse the same implementation for both of them. The
GC> > idea was that we could add write_as_xml() (or maybe write_as_json() or
GC> > whatever) later relatively easily, if needed.
GC> 
GC> Yes, I realize that now. At first, I wondered why functions like
GC> writer::write_values() were implemented in each of two namespaces.
GC> That does make it harder to search for a particular one. I guess
GC> I'm used to it now, but for a while I was thinking life would be
GC> easier if the implementations had a redundant tag, thus:
GC>   binary_format::writer::write_values()
GC>     text_format::writer::write_values()

 I didn't think of doing it like this and this would result in duplicating
the function declaration (as it would need to be declared inside the
namespace and then be defined outside of it), but maybe this would be
compensated by making the code more clear... I'm really not sure.

GC> I initially thought I'd find a common base class, with these
GC> two functions being virtual. But maybe the things in different
GC> namespaces aren't similar enough for polymorphism.

 We could force the two versions into a single class hierarchy but we don't
really need it here, there is no need for run-time dispatching, so it
looked unnecessary.

GC> It still seems odd, though, to have do_write() but not do_read().

 True, but this is often like this, just think about the classic (bad) OO
example with shapes of different kinds: it's simple to write a virtual
Write() method implemented by each shape in the hierarchy, but reading them
is a different matter... Here I think it really wouldn't help the clarity
of code to try to fold read_from_{binary,text}() into a single do_read(),
they're too different.


GC> > The only problem is that the error message won't clearly say
GC> > that the problem is due to a mismatch between the number of decimals
GC> > specified and the precision actually used for the values, but as long as 
we
GC> > suppose that this is never going to happen anyhow, the quality of error
GC> > message is probably not very important.
GC> 
GC> Take a look--I think the code I just committed manages to do that.

 Yes, sorry, I got confused, if we do the check in validate() we can indeed
give the appropriate error message, I've been thinking about an alternative
of doing it in some code specific to reading binary input, but forgot to
mention this in my previous reply, sorry.

GC> Doing it there, instead of in writer::write_values() (in the "text"
GC> namespace IIRC) as in my original sketch, also brought the very
GC> welcome incidental benefit that I didn't have to deal with objects
GC> of type 'boost::optional const&'. I realize it's awfully late for
GC> code review, but what was the rationale for 'optional'?

 I like it. I'd even go as far as saying that I love it. I think it makes
the code much clear and robust and I try to never use empty strings or
integers with special sentinel values of 0 or -1 in any new code I write.

GC> I realize
GC> that some fields such as "Construction method:" may be absent, but
GC> for text fields like that, wouldn't an empty string do as well? Or
GC> were any non-text fields missing--e.g., "Maximum select age:" for
GC> a table that is not select (so that the field wouldn't really have
GC> any meaning--but I thought it had a required value nonetheless)?

 There is a technical advantage to using optional<> too: it allows to write
template write() method that works for both strings and numbers. But the
main advantage is really that it clearly indicates that a field may or not
be present in a way that an empty string never could.

 I'm pretty sure that if you tried to rewrite the code without using
optional<> but keeping its functionality (i.e. the ability to detect
missing/duplicated fields in the input), you'd end up with a version that
would be not only longer but also much less clear.

GC> BTW, I looked over my latest change before committing it, and saw
GC> that I had written
GC>   *hash_value_ = compute_hash_value();
GC> with an asterisk, where a few lines lower you had
GC>   hash_value_ = correct_hash_value;
GC> with no asterisk. Now, hash_value_ is 'boost::optional<uint32_t>',
GC> so '*hash_value_' invokes 'T& operator*()' and we can assign a T
GC> through that reference. But how does the line with no asterisk
GC> work? The RHS of the assignment is 'correct_hash_value':
GC>   auto const correct_hash_value = compute_hash_value();
GC> and because it's auto we look up the function:
GC>   unsigned long table_impl::compute_hash_value() const
GC> and find that the type is 'unsigned long'. Oh, wait, I see:
GC>   optional& operator=(T const&)
GC> makes it work without an asterisk. But...was it really a good
GC> design decision for boost to provide that assignment operator?

 It's very convenient and std::optional<> in C++17 behaves in the same way,
so whatever we may think about it, we'll have to live with this. Personally
I don't think it's a problem.

GC> Even though it seems convenient, isn't it astonishing that these
GC> two statements:
GC>   *p = 42;
GC>    p = 42;
GC> are equivalent?

 They're not quite equivalent, of course. The first one only works if "p"
already contains something, while the second one always does. And this is
why the second is so convenient: you often want to either initialize or
update a variable, without caring about its old value.

 Regards,
VZ