[bind10-dev] A plea for Name() rather than Name(".")

[Shane Kerr]

Michael,

On Tue, 2010-02-16 at 05:25 -0600, Michael Graff wrote:
> At the last f2f meeting I really wanted to have a way to create but not
> initialize a Name class.  This was because at the time of creating it I
> did not yet know what I wanted there.  I was told this is a bogus
> request and we should not need this, because it would cause problems in
> rendering (exception) and the class should at all times be consistent.
> 
> I believe setting a Name to a default, dummy value is bad practice.  It
> allows us to let what is effectively an unset value sneak through.
> Rather than getting that exception, we instead get no error.  This means
> we could render to wire or text "." rather than throwing an exception,
> which seems to be to be against good practice.
> 
> While I'm picking on Name here, the same could be said for RRset.
> Sometimes I want a container that is not yet storing anything, but will
> once I get around to populating it.

(Skip to the end for the solution, or read through for discussion of the
issues.)

To reiterate the discussion from the face to face meeting, allowing a
class to instantiate with an invalid value is... problematic. What does
it mean to have a Name()?

Define Name() Semantics: Use Existing Relations
-----------------------------------------------
We could define the meaning of name comparison of an "empty" name versus
a real name based on existing semantics. So, maybe we would just say
that Name() is always COMMONANCESTOR when compared, and perhaps that all
boolean comparisons (<, ==, >) return false.
 
While this is possible, subtle definitions will lead to subtle bugs.

An example of how such definitions can cause confusion is with NULL in
SQL:

sqlite> select 1 where 1 = 1;
1
sqlite> select 1 where 1 = 2;
sqlite> select 1 where 1 = NULL;
sqlite> select 1 where NULL = NULL;
sqlite> select 1 where NULL is NULL;
1

NULL compared to anything is NULL - even when compared to itself. This
can cause strange results, as per the following example:

sqlite> create table foo ( a int );
sqlite> insert into foo values (1);
sqlite> insert into foo values (NULL);
sqlite> insert into foo values (2);
sqlite> create table bar ( b int );
sqlite> insert into bar values (1);
sqlite> insert into bar values (NULL);
sqlite> insert into bar values (2);
sqlite> select * from foo, bar where foo.a=bar.b;
1|1
2|2

Even though both tables have 3 rows, our join only retrieves 2 rows. I
assure you this "feature" of SQL does cause real-world problems. :)

Plus, we would need to define a wire format, a text format, and the
semantics for concatenate(), in a way that made some sense (if you think
maybe we should throw an exception in one or more of these cases, see
below for a discussion of that).


Define Name() Semantics: Create a New Relation
----------------------------------------------
We could also create a new NameRelation of something like EMPTYNAME.

This would allow us to explicitly check for this condition. There are a
couple of reasons why this is not ideal.

First, it will complicate all clients using the class. Now they need to
consider what their class or application should do when presented with
an empty name. So, right now I can say:

    if (a->compare(b) != COMMONANCESTOR) {
        // do something knowing that a contains b (or vice versa)

However, with EMPTYNAME I would have to say:

    result = a->compare(b);
    if ((result != COMMONANCESTOR) && (result != EMPTYNAME)) {
        // do something knowing that a contains b (or vice versa)

This is a trivial example, but this kind of logic will have to appear
everywhere throughout all client code.

The second, minor, reason that this is not ideal is that we aren't
capturing all the possible combinations. So, EMPTYNAME doesn't tell us
which of the two compared names is empty. This can be worked around by
adding an isEmpty() method of course.

Also, all the definitions before still need to be done (wire format,
text format, ...).


Declare Name() Use to Be an Error
---------------------------------
Another possibility is to simply state that trying to compare an "empty"
name is a bug. So any place where you try to do something other than
copy to the Name or use the isEmpty() method you are confused and doing
the wrong thing, so we throw an exception.

The problem with this is that anyone writing a function then has to be
prepared for exceptions in places that they did not have to be before.

So, I could have:

    void
    example(const Name& n)
    {
        // do some work
        // get some resources
        NameComparisonResult cmp = fromitz.compare(n);
        if ((cmp != SUPERDOMAIN) && (cmp != EQUAL)) {
            // do something
        }
        // do some more work
        // free up resources
    }

If you use an exception to indicate an "empty" name, then all accesses
to a name will have to be written with the possibility of exceptions in
mind. This seems both dangerous and complicated to me, especially when
the other option is...


Use a Pointer
-------------
Just use a pointer. Really.

    Name* n = NULL;
      .
      .
      .
    while (some_condition) {
        if (n == NULL) {
            // hey, "empty" name!
        } else {
            // non-"empty" name
        }
    }

You can use shared pointers too:

    boost::shared_ptr<Name> n;
      .
      .
      .
    while (some_condition) {
        if (n.get() == NULL) {
            // "empty" name, in a safer way
        } else {
            // non-"empty" name, with seatbelts securely fastened
        }
    }

Shared pointers may not be necessary if you can be sure that your code
is exception-safe, otherwise probably are a good idea, to insure that
things get cleaned up properly. They can also let you be a bit lazier,
which is always good.

--
Shane