BIND 10 #2096: Define and implement "RdataReader" class
BIND 10 Development
do-not-reply at isc.org
Fri Aug 24 00:34:57 UTC 2012
#2096: Define and implement "RdataReader" class
-------------------------------------+-------------------------------------
Reporter: | Owner: jinmei
jinmei | Status: reviewing
Type: task | Milestone:
Priority: | Sprint-20120904
medium | Resolution:
Component: data | Sensitive: 0
source | Sub-Project: DNS
Keywords: | Estimated Difficulty: 5
Defect Severity: N/A | Total Hours: 0
Feature Depending on Ticket: |
scalable inmemory |
Add Hours to Ticket: 0 |
Internal?: 0 |
-------------------------------------+-------------------------------------
Comment (by jinmei):
Replying to [comment:18 vorner]:
The revised version looks almost okay for merge. A couple of possible
points:
- it may be better to call nextInternal() directly from iterate()
and iterateRdata() to skip the call redundancy. it may be marginal
or the compiler may optimize it anyway, but quick benchmark shows
it's actually slightly faster. or we may define next() inside
the class definition to enforce inlining.
- in iterateSingleSig(), NO_BOUNDARY should be an impossible case.
should we assert() it?
And some other notes:
- Not sure if you checked that, so: Is the revised version of
- getSize() okay?
- I made a few more cleanup fixes.
- I realized I forgot to push some intermediate commits of mine,
including the benchmark stuff (I did it again...). Because of that
I encountered some conflicts at the time of pull and needed to
resolve them. The conflict was mainly because of the commit
04c050c. I hope I didn't break anything as a result of the conflict
resolution, but it would be nice if you can check it.
- I've revised the benchmark code so it only uses the current
implementation. If you are interested in the comparison I did,
you can do it at the 220c3ff version.
Some of the responses below may result in a bit more changes to the
branch, but I'd leave the decision to you.
> > The "next" version is also less efficient. It's inherently so,
> I'm really surprised about the 10%. Did you use a function, or a
functor/boost::bind object?
Functor (boost::bind). You can see it in the now-pushed benchmark
code.
> If C++ could do lambda functions in some reasonable way (and, no,
functors are not reasonable), I wouldn't hesitate a moment and used the
callback version. But if you look at the code in the tests, especially the
ones where I need to switch the renderer in the middle of the use of a
reader, I'm still not sure this is readable.
I don't deny in some cases the indirect nature of callback can make
the code less readable, and regarding readability it could merely be a
matter of taste. But I'd like to say that the "switch renderer"
example is not a good one to discuss readability.
> > - I'm not really sure about the meaning of 'private':
> > {{{#!cpp
> > /// This header should be considered private to the implementation and
should
> > /// not be used from outside.
> > }}}
>
> I meant people should not include it directly. Another option would be
placing both the reader and encoder in the same file and hide it
completely. Should I do it?
Hmm, in that case maybe we should add an '#error' directive to catch
the misuse. Continuing to the next topic:
> > if it means it's private to `RdataEncoder` and `RdataReader` only,
we should
> > at least extract `RdataNameAttributes` to somewhere else because
> > it's expected to be used by other classes via the reader's name
> > action. Or, if it's about naming (rdata_encoder.h looks like
> > encoder specific), maybe we should introduce more generic name like
> > "rdata_serialization.h/cc" and define related things there?
> > (and still hide implementation details in .cc). On a related point,
> > we might want to rename `RdataEncoder` to `RdataSerializer`?
>
> The attributes are available, as the header is included indirectly. Is
that a problem?
Hmm, I'm afraid the indirect relationship isn't very clear, and we can
easily tend to include rdata_field.h directly just to use
RdataNameAttributes values.
I'd also like to hide things like `RdataFieldSpec` or
`getRdataEncodeSpec` as much as possible.
> Also, what is the advantage of Serializer over Encoder?
Not much, but as you renamed the encoder test file to
rdata_serialization_unittest for testing both encoder and reader, I
thought you perhaps didn't like to define "reader" related things in
something named "encoder" but the term "serialization" sounded okay.
The "er" part of "serializer" may make it not so different from
"encoder" in that sense, though.
Anyway, considering these, my latest personal suggestion would be:
- create a file rdata_serialization.h/cc
- in the header file, only define "public" things:
`RdataNameAttributes` and the two classes (and perhaps a bit more)
- everything else is hidden in the .cc
But I'd leave the decision to you on this point. These are basically
intended for internal use only anyway, so disclosing details may be
less substantial.
> [ Bunch of Result related stuff that is irrelevant now ]
>
> > - `RdataReader` constructor: as suggested above, I'd change the type
> > of 'data' to `const void*`. Then, I'd use reinterpret_cast for
> > lengths_:
> > {{{#!cpp
> > lengths_(reinterpret_cast<const uint16_t*>(data)),
> > }}}
>
> Changed, but I'm not sure about the reinterpret_cast. I believe that one
is one step more dangerous than we do. While we look at the data pointed
to as a different type, the reinterpret_cast looks at the pointer itself
as a different type.
Hmm, I'm afraid I don't understand what the last sentence "while
we..." means, but in my understanding of reinterpret_cast this case is
exactly one where it's intended to be used, like for the reason I
explained in my previous comment. Also, "The C++ Programming
Language" book says:
"The static_cast operator converts between related types such as one
pointer type to another in the same class hierarchy... The
reinterpret_cast handles conversion between unrelated types such
as... or a pointer to an unrelated pointer type".
Superficially, void and uint16_t are in the same class hierarchy, so
for the above specific conversion we could use static_cast (and in
fact compilers won't complain), but in effect we are converting (e.g.)
`RdataSet*` to `uint16*`, and they are unrelated pointer types.
> > - Same for `data_`.
>
> You mean the conversion, or that `data_` itself should be void*? The
second would be very inconvenient, since we need to move the offset in it.
I meant the conversion. And the revised code is okay.
> Replying to [comment:17 jinmei]:
> > - rdata_reader.h: this file can (possibly) be included outside of this
> > directly (without explicit "-I"), rdata_field.h should be specified
> > as a relative path:
> > {{{#!cpp
> > //#include "rdata_field.h"
> > #include <datasrc/memory/rdata_field.h>
> > }}}
>
> In fact, that is the exact reason why the quoted version is correct. The
pointed bracket version uses the include path to search for first matching
file and takes the `-I` switches into account. This is potentially
dangerous, because there may be a matching file sooner in the includes.
The quoted version is always relative to the file where the `#include`
statement is written and -I are used when it is not found in the correct
directory. In this case, this is relative to the `rdata_reader.h` file, no
matter where that one is included from.
Ah, okay. It was in fact my misunderstanding. Also, in my benchmark
I copied rdata_reader.h to benchmarks/ subdirectory and saw a compiler
error due to "rdata_field.h", which made me make this comment. I'm
now okay with the current version.
--
Ticket URL: <http://bind10.isc.org/ticket/2096#comment:19>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list