Hi all,
I have found a bug in the generated code for the Smalltalk thrift
interface.
I used svn revision:
mig...@laptop:~/thrift-svn$ svn info
Ruta: .
URL: http://svn.apache.org/repos/asf/incubator/thrift/trunk
Raíz del repositorio: http://svn.apache.org/repos/asf
UUID del repositorio: 13f79535-47bb-0310-9956-ffa450edef68
Revisión: 888521
Tipo de nodo: directorio
Agendado: normal
Autor del último cambio: todd
Revisión del último cambio: 887812
Fecha de último cambio: 2009-12-06 18:42:38 -0600 (dom 06 de dic de
2009)
I did:
bootstrap.sh
configure
make
and then I tried to load the thrift smalltalk code from
lib/st/thrift.st
in a squeak Squeak3.10.2-7179-basic.zip, pristine image from squeak.org.
That halted with an error about a duplicated readString method
declaration. The error is fixed in the attached patch.
Next I compiled the code for the cassandra thrift:
https://svn.apache.org/repos/asf/incubator/cassandra/trunk/interface/cassandra.thrift
with the command:
./compiler/cpp/thrift --gen st cassandra.thrift
and tried to file in the generated code
gen-st/cassandra.st
in said image.
Again the load halted. This time because of the underscores in the
generated code for the cassandra thrift definition. Squeak (and derived
smalltalks as Pharo) doesn't allow underscores in variable names (the
underscore is a legacy character used for variable assigment). Anyway I
have modified the code for smalltalk generator:
compiler/cpp/src/generate/t_st_generator.cc
so that when a variable name in the thrift definition has a "_"
character, this is removed and the next character is converted to
uppercase. e.g. a variable like column_name will be generated in
smalltalk code as columnName. This way the code loads (and works
correctly) on Squeak.
The generated code has been tested in the following smalltalks:
Squeak3.10.2-7179-basic.image
PharoCore-1.0-10491rc1.image
and in both of them the new generated code for cassandra works
correctly.
I haven't tested the code generation with a thrift spec other than
cassandra, so maybe this will introduce some bugs. For my case it worked
great.
If anyone is using cassandra and want to test it, I installed the
default cassandra server on localhost port 9160 and tested from Squeak
and from Pharo by loading
lib/st/thrift.st
gen-st/cassandra.st
Then in a Squeak or Pharo workspace:
"Insert 10000 values"
[| cp result client |
client := CassandraClient binaryOnHost: 'localhost' port: 9160.
cp := ColumnPath new
columnFamily: 'Standard1';
column: 'col1'.
1 to: 10000 do: [ :i |
result := client insertKeyspace: 'Keyspace1'
key: 'row', i asString
columnPath: cp
value: 'v', i asString
timestamp: 1
consistencyLevel: ((Cassandra enums at: 'ConsistencyLevel') at:
'QUORUM').]] timeToRun
And read the values just inserted:
"Read 10000 values"
[| cp result client |
client := CassandraClient binaryOnHost: 'localhost' port: 9160.
cp := ColumnPath new
columnFamily: 'Standard1';
column: 'col1'.
1 to: 10000 do: [ :i |
result := client getKeyspace: 'Keyspace1'
key: 'row', i asString
columnPath: cp
consistencyLevel: ((Cassandra enums at: 'ConsistencyLevel') at:
'QUORUM').]] timeToRun
Regarding the patch, I am not a C++ expert and the drop_underscores()
function surely can be improved by someone with more experience with C++
idioms. Essentially what it does is to convert:
from -> to
----- -----
a_variable -> aVariable
a_new_var -> aNewVar
column_path -> columnPath
variable -> variable
Give it a try and if deemed good enough please add it to the thrift
code.
Those changes are published under the apache public license, the same as
the thrift codebase.
Also, I just subscribed to the mailing list and saw the JIRA bug
tracking system. I tried to search for a related ticket but (apart for
the not very user-friendly interface) can't find anything. Also tried to
add an issue for the fix but asked me to register. I didn't follow that
path. If the rules for the project are to add a issue in JIRA I could
give a try again, but I prefer not to (I don't want to have other
user/password to remember).
The patch is generated with
svn diff -x -u > patch.888520
against revision 888520 of the thrift svn code.
Comments, directions, criticisms are welcome.
Cheers,
--
Miguel Cobá
http://miguel.leugim.com.mx
Index: lib/st/thrift.st
===================================================================
--- lib/st/thrift.st (revisión: 888520)
+++ lib/st/thrift.st (copia de trabajo)
@@ -294,7 +294,6 @@
!TBinaryProtocol methodsFor: 'reading' stamp: 'pc 02/07/2009 19:00'!
readString
-readString
| sz |
sz := self readI32.
^ sz > 0 ifTrue: [(transport read: sz) asString] ifFalse: ['']! !
Index: compiler/cpp/src/generate/t_st_generator.cc
===================================================================
--- compiler/cpp/src/generate/t_st_generator.cc (revisión: 888520)
+++ compiler/cpp/src/generate/t_st_generator.cc (copia de trabajo)
@@ -120,6 +120,9 @@
void st_getter(std::ofstream &out, std::string cls, std::string name);
void st_accessors(std::ofstream &out, std::string cls, std::string name, std::string type);
+
+ std::string drop_underscores(std::string str);
+
std::string class_name();
std::string client_class_name();
std::string prefix(std::string name);
@@ -706,7 +709,7 @@
for (fld_iter = fields.begin(); fld_iter != fields.end(); ++fld_iter) {
bool optional = (*fld_iter)->get_req() == t_field::T_OPTIONAL;
- string fname = (*fld_iter)->get_name();
+ string fname = drop_underscores((*fld_iter)->get_name());
string accessor = sname + " " + sanitize(fname);
if (optional) {
@@ -834,6 +837,27 @@
}
}
+// We found all the occurrencies of _ in names and delete them.
+// Also, the character to the right of the _ is changed to uppercase
+// so some_value becomes someValue
+string t_st_generator::drop_underscores(string str) {
+ size_t pos;
+ string before;
+ string after;
+ string new_string = str;
+
+ while((pos = new_string.find("_")) != string::npos) {
+ // The string before _
+ before = new_string.substr(0, pos);
+ // The string after _
+ after = new_string.substr(pos + 1);
+ // A new string without a _
+ new_string = before + capitalize(after);
+ }
+
+ return new_string;
+}
+
void t_st_generator::generate_send_method(t_function* function) {
string funname = function->get_name();
string signature = function_signature(function);
@@ -854,10 +878,10 @@
indent_down();
f_ << indent() << "oprot writeStructBegin: " <<
- "(TStruct new name: '" + capitalize(function->get_name()) + "_args')." << endl;
+ "(TStruct new name: '" + capitalize(drop_underscores(funname)) + "_args')." << endl;
for (fld_iter = fields.begin(); fld_iter != fields.end(); ++fld_iter) {
- string fname = (*fld_iter)->get_name();
+ string fname = drop_underscores((*fld_iter)->get_name());
f_ << indent() << "oprot writeFieldBegin: (TField new name: '" << fname <<
"'; type: " << type_to_enum((*fld_iter)->get_type()) <<
@@ -873,9 +897,11 @@
st_close_method(f_);
}
+
+
// We only support receiving TResult structures (so this won't work on the server side)
void t_st_generator::generate_recv_method(t_function* function) {
- string funname = function->get_name();
+ string funname = drop_underscores(function->get_name());
string signature = function_signature(function);
t_struct result(program_, "TResult");
@@ -911,7 +937,7 @@
out << "\"";
for (f_iter = fields.begin(); f_iter != fields.end(); ++f_iter) {
- out << (*f_iter)->get_name() << ": " << type_name((*f_iter)->get_type());
+ out << drop_underscores((*f_iter)->get_name()) << ": " << type_name((*f_iter)->get_type());
if ((f_iter + 1) != fields.end()) {
out << ", ";
}
@@ -945,7 +971,7 @@
"\tcategory: '" << generated_category() << "'!\n\n";
for (f_iter = functions.begin(); f_iter != functions.end(); ++f_iter) {
- string funname = (*f_iter)->get_name();
+ string funname = drop_underscores((*f_iter)->get_name());
string signature = function_signature(*f_iter);
st_method(f_, client_class_name(), signature);
@@ -992,7 +1018,7 @@
* @return String of rendered function definition
*/
string t_st_generator::function_signature(t_function* tfunction) {
- return tfunction->get_name() + capitalize(argument_list(tfunction->get_arglist()));
+ return drop_underscores(tfunction->get_name()) + capitalize(argument_list(tfunction->get_arglist()));
}
/**
@@ -1010,7 +1036,8 @@
} else {
result += " ";
}
- result += (*f_iter)->get_name() + ": " + (*f_iter)->get_name();
+ string name = drop_underscores((*f_iter)->get_name());
+ result += name + ": " + name;
}
return result;
}
