Closed mdavidsaver closed 6 years ago
+1 Looks extremely attractive.
My naming suggestions/comments:
build()
at the end of configuring a FieldBuilder object to tell it to go do its thing, not at the beginning to create it — building a builder seems rather meta. I'd like to replace build()
with something else, maybe newBuilder()
or New()
although those might be a bit meh. I considered newStructure()
which neatly wraps the add()
calls; the name is rather close to createStructure()
linguistically, but not too bad. The names start()
or create()
might also work here if you prefer them.createStructure()
is very descriptive, I'd suggest taking the lead from that and change create()
to become createInstance()
(or newInstance()
) which says exactly what it does.With these changes your example becomes something like:
PVStructurePtr value(FieldBuilder::New()
->add("a", pvInt)
->add("b", pvDouble)
->add("c", pvString)
->createStructure()->createInstance());
Looking at FieldBuilder::New()
my first thought is "New what? Field? Builder?" Doesn't really convince me.
IMHO, such "natural" APIs should be regular to be comprehensive. E.g. for building complex things, the method names should be verb+noun, with a matching pair of verbs at both ends. Verb pairs could be begin
/end
or start
/finish
.
PVStructurePtr value(FieldBuilder::beginStructure()
->add("a", pvInt)
->add("b", pvDouble)
->add("c", pvString)
->endStructure()->createInstance());
Even building complex structures could look pretty obvious.
PVStructurePtr value(FieldBuilder::beginStructure()
->add("a", pvInt)
->beginStructure()
->add("xx", pvDouble)
->add("yy", pvDouble)
->endStructure()
->add("c", pvString)
->endStructure()->createInstance());
Could be extended with beginUnion()
/ endUnion()
etc.
Dreaming on: you could even define structures defineStructure("foo")
/ endStructure()
to add them to the builder's catalog, and later on just say addStructure("foo")
to pull them in.
I remember discussing such a natural API for building structures (in Java) with @gcarcassi about 8 years ago, and the result looked pretty nice - Gabriele, do you remember what this was for? Calling the same things by the same names would be nice.
Yes, it was for IRMIS. The idea was all the main objects where immutable, so you could pass them around in a multithreaded environment without issue. Then you would create an "updateSet", which is basically a fluent API for a database transaction that then was shipped to the server side. The idea was also that the API was really a copy on write, so that you could keep "checkpoints". All with being memory efficient.
I always thought that something like that would be useful for pvData as well. I want to be able to pass around structures and values in a multithreaded environment without worry of synchronization. I want to keep a previous snapshot (e.g. value update from the network comes while I am updating the UI). I want to update only a part of a large array without a full copy. I want to create a new structure that looks very similar to another, etc...
I like Ralph's API suggestion. Looks very intuitive and easy to use.
Dreaming on
This is getting out of scope for what I had in mind for this iteration. This PR is about adding two new shorthand methods. FieldBuilder::beginStructure()
seems neat, but requires more changes then I'm personally looking to make at present. I'm not going to insist that you guys stay inside this box, but please stay within sight of the box for this PR.
I dislike FieldBuilder::New()
as this would be (I think) the only method name beginning with a capital, and not one I've encountered in a C++ API before. Field::createInstance()
seems unnecessarily verbose. There is only one thing a Field
can create.
Completely agree that this kind of lucid dreams are outside the scope of this issue.
This particular one was triggered by your statement at the last telecon that the Builder knows which structures have been built and returns the existing one if you try to re-build a structure. Conceptually this is the core of a structure catalog, and having an API to access it would be another (yes, separate) step. I'm (over) enthusiastic because this is very much what I had in mind for the normative types helpers back in the validator/builder discussion. Here the NT Builder becomes "just" a Builder with a preloaded catalog, in this case of the NT structures. (I know, optional fields will add an awful layer of complexity.) Having previous NT structure versions in the catalog would make it compatible for older applications that want to use last year's NT type. Allowing to preload the Builder catalog from an XML representation of named structures would make adding site specific complex types nice and easy. Last not least: the existing functionality (by your statement) of checking a new structure against the existing ones is the core of a validator. [Time to review some 60 page cubicle wiring diagrams before I get too excited.]
Note that the following example also adds standard fields
PVStructurePtr value(FieldBuilder::build()
->add("timeStamp",standardField->timeStamp())
->add("alarm",standardField->alarm())
->add("a", pvInt)
->add("b", pvDouble)
->add("c", pvString)
->createStructure()->create());
Note that for standard fields a single instance of the introspection structure is used.
Love the idea of XML representation of syntax, but wouldn’t it then be XML Schema? Actually specifically XSD, please not DTD or Relax-NG or any quasi schema definition languages.
Schema validation would be a brilliant asset for pvStructures.
On Oct 4, 2018, at 11:58 PM, Ralph Lange notifications@github.com wrote:
Completely agree that this kind of lucid dreams are outside the scope of this issue.
This particular one was triggered by your statement at the last telecon that the Builder knows which structures have been built and returns the existing one if you try to re-build a structure. Conceptually this is the core of a structure catalog, and having an API to access it would be another (yes, separate) step. I'm (over) enthusiastic because this is very much what I had in mind for the normative types helpers back in the validator/builder discussion. Here the NT Builder becomes "just" a Builder with a preloaded catalog, in this case of the NT structures. (I know, optional fields will add an awful layer of complexity.) Having previous NT structure versions in the catalog would make it compatible for older applications that want to use last year's NT type. Allowing to preload the Builder catalog from an XML representation of named structures would make adding site specific complex types nice and easy. Last not least: the existing functionality (by your statement) of checking a new structure against the existing ones is the core of a validator. [Time to review some 60 page cubicle wiring diagrams before I get too excited.]
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.
So to reset this discussion back to my original comments, which were just suggestions for renames within the context of the existing PR:
I dislike
FieldBuilder::New()
as this would be (I think) the only method name beginning with a capital, and not one I've encountered in a C++ API before.
That's a reasonable objection, what about my other suggestions? I still claim that a build()
method call should appear at the end of a call-chain that starts with FieldBuilder
, not at the beginning.
Field::createInstance()
seems unnecessarily verbose. There is only one thing aField
can create.
You know that, but this call-chain tends to stick out of code that uses it, and I was suggesting that explicit might be better than implicit in this particular case. The FieldBuilder methods for creating arrays, structures and unions are equally verbosely named (and necessarily so). I'd be okay with newInstance()
, buildInstance()
or even just build()
here as there are no other methods used with build in their names (see above about what a FieldBuilder does, even though this isn't actually a FieldBuilder method).
@anjohnson How about FieldBuilder::begin()
and Field::build()
? eg.
PVStructurePtr value(FieldBuilder::begin()
->add("a", pvInt)
->add("b", pvDouble)
->add("c", pvString)
->createStructure()->build());
And bare in mind that this example is a bit contrived. It is not usual to define and instantiate in one shot. Realistic (and efficient) usage separates these steps.
// once
StructureConstPtr mytype(FieldBuilder::begin()
->add("a", pvInt)
->add("b", pvDouble)
->add("c", pvString)
->createStructure());
...
// many times
PVStructurePtr inst(mytype->build());
@mdavidsaver That looks good to me. :+1:
The method names are changed, and the paint is drying.
Boilerplate reduction for using FieldBuilder and creating PVField instances. Turns:
into:
I've long been annoyed at how verbose Structure creation is. Hearing @shroffk complaining about the same, and suggesting something like this has motivated me to do something about it.