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

[Vadim Zeitlin]

On Wed, 14 Dec 2016 14:50:23 +0000 Greg Chicares <address@hidden> wrote:

GC> The patch below [0] shows what I'm trying to accomplish: given a flawed
GC> historical binary file whose values are inconsistent with its stated
GC> number of decimals, write a text file that states the number of decimals
GC> that seems to have been intended and formats values to that corrected
GC> precision. Everything was fine until I realized that these corrections
GC> require the hash value to be recalculated, so I added this line:
GC> 
GC> +        hash_value_ = compute_hash_value();
GC> 
GC> in the patch below, but that line doesn't compile: both hash_value_ and
GC> compute_hash_value() are hidden from this context.
GC> 
GC> I suppose this means I should move these changes from the vicinity of
GC> text_format::writer::write_values(), probably to table_impl::do_write().

 This could work, but I'd actually prefer to do this on reading the table,
not when writing it. For me logically adjusting the number of decimals used
is part of validation and so should be done as early as possible, i.e. in
validate().

GC> Then the number of decimals and hash value would be recalculated in
GC>   text_format::writer::write_values()
GC> which is what's really wanted, and also, accidentally, in
GC>   binary_format::writer::write_values()
GC> which isn't desired but might be okay (although I'm not sure whether
GC> that might cause hash values to differ between binary and text).

 The hash value does depend on the number of decimals used, indirectly
(i.e. it doesn't include its value itself in the text that it computes the
CRC of, but it still depends on it because it affects the values which are
part of this text). And this is, IMHO, another reason to do this adjustment
in validate(), so that it could mesh with the hash value check correctly.

GC> Or
GC> maybe the right place for this change is
GC>     soa_v3_format::table_impl::write_as_text()
GC> so that it would be excluded from
GC>     soa_v3_format::table_impl::write_as_binary()
GC> but that breaks the implicit assumption that those two functions
GC> share the same implementation.

 I think it's nice to reuse the same implementation for both of them. The
idea was that we could add write_as_xml() (or maybe write_as_json() or
whatever) later relatively easily, if needed.


GC> But there's another possible problem. I had imagined that changing the
GC> number of decimals could affect only write operations, but its value
GC> is used in some read operations as well. In parse_single_value(), it's
GC> used to set 'num_spaces_allowed', which is used only for validation;
GC> there, I'd hesitate to "fix" it, because that might prevent reading
GC> a table. It's also used here:
GC> 
GC>                 // After the max select age only the last column remains, 
just
GC>                 // skip the spaces until it.
GC>                 skip_spaces
GC>                     
(*select_period_*text_format::get_value_width(*num_decimals_)
GC>                     ,start
GC>                     ,current
GC>                     ,line_num
GC>                     );
GC> 
GC> and would the same reasoning apply there? I suppose that serves a
GC> validation purpose, too: if the stated number of decimals is not
GC> consistent with the format of the text table being read, then
GC> that might indicate a serious problem (a different problem than the
GC> one addressed by the patch below, which tries to fix an inconsistency
GC> between the stated number of decimals and binary values).

 If we assume that the number of decimals in the text format always
corresponds to the reality, then it shouldn't matter because it's never
going to be adjusted anyhow in this case then. If we do encounter a text
file with mismatching number of decimals, then it will result in an error
when trying to read it -- but this would seem like the right thing to do,
wouldn't it? The only problem is that the error message won't clearly say
that the problem is due to a mismatch between the number of decimals
specified and the precision actually used for the values, but as long as we
suppose that this is never going to happen anyhow, the quality of error
message is probably not very important.


GC> Vadim, where do you think I should make this change?

 In validate().

 As always, please let me know if you'd like me to make a patch based on
your changes doing this or if you, as I suspect, prefer to do it yourself.

 Thanks,
VZ