Starting this now, as it's blocking something else I already have in progress.

Russell, could you take a look at this? Basically, we're changing the delimiter used in afw::table fields from '.' to '_', with the transition corresponding to a "version" attribute held by the Schema. I also removed a workaround in the slot definition in SourceTable that was made unnecessary by these changes.

Some of the changes are surrounded by complex code (particularly in Schema.cc) but are themselves quite simple. The changes also look larger than they are in the git summary because I moved a pair of function implementations from Source.h to Source.cc in the course of removing the workaround.

Finally, the git diff is a bit unusual because I started work on this issue on top of a branch from another issue (DM-958) which is also out for review. The git diff command below will give you the changes that correspond to just this issue.

Despite all of the above, I think this should be a pretty easy review.

afw:u/jbosch/DM-1058 % git diff --stat u/jbosch/DM-958...u/jbosch/DM-1058

 include/lsst/afw/table/Source.h.m4 |  100 +++---------------------------------

 src/table/Schema.cc                |   41 +++++++++------

 src/table/Source.cc                |   84 ++++++++++++++++++++++++++++++

 tests/testSchema.py                |   15 ++++++

 tests/ticketDM-433.py              |   15 +++---

 5 files changed, 138 insertions(+), 117 deletions(-)

This looks great.

As warned, I had a little trouble figuring out what to compare to what in SourceTree, but the list of files helped and I think I have it close enough.

Minor points about Schema.cc:

Schema.cc: Why did you implement two constructors for Schema instead of one constructor with the version parameter defaulted to DEFAULT_VERSION? Using one would reduce code.

Schema.cc line 617:
int const Schema::DEFAULT_VERSION
What does this do? I'm guessing it pulls DEFAULT_VERSION into the current namespace, but if so, why bother?
Why not just use Schema::DEFAULT_VERSION in the one place it is used (a few lines later).
(If you do decide to use one constructor, then the default value can be set in the .h file, which avoids the need to use the constant in the .cc file).

Schema.cc line 200: please consider moving the constructor before the operator. I think it would makes the code a bit clearer (based on some initial confusion on my part).

Source.h.m4: moving the subroutine code seems like a good idea, and the new non-version=0 code is nicely simplified, though as warned, it made the changes harder to follow.

Thanks for the thorough review. A few replies below:

Schema.cc: Why did you implement two constructors for Schema instead of one constructor with the version parameter defaulted to DEFAULT_VERSION? Using one would reduce code.

I was a little worried about whether it was completely safe to use a class-static variable as a default value for an argument. I know you can run into problems in C++ when you initialize one static variable using another (http://www.parashift.com/c++-faq-lite/static-init-order.html), and I think default arguments are treated similarly to static variables. If you're pretty certain this is safe, I'd be happy to change it.

Schema.cc line 617:
int const Schema::DEFAULT_VERSION
What does this do? I'm guessing it pulls DEFAULT_VERSION into the current namespace, but if so, why bother?
Why not just use Schema::DEFAULT_VERSION in the one place it is used (a few lines later).
(If you do decide to use one constructor, then the default value can be set in the .h file, which avoids the need to use the constant in the .cc file).

When you initialize a const static int data member in the class declaration, it's not quite a complete declaration; you can use it in most contexts, but you can't take it's address, because C++ doesn't know what object file to compile that address into, as it would you initialize a static data member in a .cc file, which is what this line tells it (http://stackoverflow.com/questions/3025997/c-defining-static-const-integer-members-in-class-definition). I've found that sometimes Swig wrappers want the address, so this sort of line is necessary.

Schema.cc line 200: please consider moving the constructor before the operator. I think it would makes the code a bit clearer (based on some initial confusion on my part).

Will do (here and in other similar places in the file). I have a bad habit of putting constructors at the bottom of class definitions instead of at the top, because I often think about them last, so I'm glad you caught this, as I do recognize that it's easier to read the class if they come first.

Regarding Schema.h: static int const DEFAULT_VERSION = 0;
I am not certain that using a static int as a default for a constructor is safe. However, is it important that this be a static constant? If so…having two constructors is no big deal, but if not, it would simplify the code a bit.

I was hoping to centralize all the usage of the default version in one place, but I'm not sure how much I've actually succeeded with that. We've got an upcoming issue (DM-1070) to change the default version, and I'll revisit the question there, as we'll need to hunt down any other places it might be used then.