Description

We're mostly using the C++ interface to thrift, and I wanted to be
able to initialize structures more easily than a whole series of lines
that set each of the parameters separately. Attached is a patch that
adds a constructor to the C++ objects that allow you to fully
initialize an object in a single call. I added two tests for this,
one in the DebugProtoTest.cpp file, and one in the
OptionalRequiredTest.cpp. The patch is against the 20080411p1 release.

In kinda_by_id_less_than, after the first two conditions, you could return abs(a->get_key()) < abs(b->get_key());
Also, what would you think about just making this a static function of the generator instead of a struct?
Also, what would you think about naming the arguments lhs and rhs instead?

Please use spaces instead of tabs for indentation.

The okay_prefix stuff is unnecessary. You can alias the constructor arguments to the field names and it works fine.

David Reiss
added a comment - 07/Oct/08 20:49 In kinda_by_id_less_than, after the first two conditions, you could return abs(a->get_key()) < abs(b->get_key());
Also, what would you think about just making this a static function of the generator instead of a struct?
Also, what would you think about naming the arguments lhs and rhs instead?
Please use spaces instead of tabs for indentation.
The okay_prefix stuff is unnecessary. You can alias the constructor arguments to the field names and it works fine.

Jérémie BORDIER
added a comment - 22/Mar/09 15:45 I feel like this issue shouldn't be fixed as is, but generalized to other languages that could benefit from this kind of API enhancement in the compiler. What do you guys think ?

I have no problem with the basic design, as-is. Some of the other languages already have similar features, and it is tough to require contributors to implement their feature in languages that they do not use.

David Reiss
added a comment - 24/Mar/09 20:15 - edited I have no problem with the basic design, as-is. Some of the other languages already have similar features, and it is tough to require contributors to implement their feature in languages that they do not use.

— V2: (assuming a series of intermediate ones with the necessary optionals)

struct point {
required 1: i32 x;
required 2: i32 y;
}

struct circle {
required 4: point center;
required 3: i32 radius;
}

The problem is that the latter circle is now initialized as circle(radius, center), which seems inverted to me from the "expected" order for defining such an object. However, I can't reuse the 1 and 2 tags because they are obsolete (as I understand the thrift migration of structure tag rules)

I'd planned to put in an option to control which way the constructors would go it, but I got busy.

Eric Anderson
added a comment - 08/Apr/09 19:27 I can't commit (no permission), and the related problem is that the patch as written isn't actually useful to us; we want the constructor argument order to match with the order in the thrift file.
consider the following evolution of a structure:
struct circle {
required 1: i32 x;
required 2: i32 y;
required 3: i32 radius;
}
— V2: (assuming a series of intermediate ones with the necessary optionals)
struct point {
required 1: i32 x;
required 2: i32 y;
}
struct circle {
required 4: point center;
required 3: i32 radius;
}
The problem is that the latter circle is now initialized as circle(radius, center), which seems inverted to me from the "expected" order for defining such an object. However, I can't reuse the 1 and 2 tags because they are obsolete (as I understand the thrift migration of structure tag rules)
I'd planned to put in an option to control which way the constructors would go it, but I got busy.
Out of curiosity, is there a reason that you're replacing \n with endl? They are exactly the same except that endl is equivalent to \n + flush output, so is strictly slower. ( http://faq.cprogramming.com/cgi-bin/smartfaq.cgi?id=1043284376&answer=1045690279 ) – we had this argument internally, and tested it on windows, \n in a string generated \x0a\x0d in a file.

The Thrift compiler no longer has any notion of the order that fields are defined in the IDL file. If this poses a problem for you, please see THRIFT-236. I switched to endl because it is the convention in the surrounding code.

David Reiss
added a comment - 08/Apr/09 19:48 The Thrift compiler no longer has any notion of the order that fields are defined in the IDL file. If this poses a problem for you, please see THRIFT-236 . I switched to endl because it is the convention in the surrounding code.

I see what your issue is here. THRIFT-236 was intended to have an effect on the serialization order, but our solution to this problem inadvertently also had an effect on things like constructors and service method parameter ordering. I tend to agree that this change is undesired and we should probably fix it.

Bryan Duxbury
added a comment - 08/Apr/09 21:58 I see what your issue is here. THRIFT-236 was intended to have an effect on the serialization order, but our solution to this problem inadvertently also had an effect on things like constructors and service method parameter ordering. I tend to agree that this change is undesired and we should probably fix it.