Skip to content

Commit

Permalink
Update optional mapping in Ruby (#2566)
Browse files Browse the repository at this point in the history
  • Loading branch information
@bernardnormier
bernardnormier authored Jul 25, 2024
1 parent a5b2848 commit bcdd6a4
Show file tree
Hide file tree
Showing 7 changed files with 216 additions and 245 deletions.
6 changes: 3 additions & 3 deletions ruby/src/IceRuby/Operation.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -403,7 +403,7 @@ IceRuby::OperationI::prepareRequest(
{
ParamInfoPtr info = *p;
volatile VALUE arg = RARRAY_AREF(args, info->pos);
if ((!info->optional || arg != Unset) && !info->type->validate(arg))
if ((!info->optional || arg != Qnil) && !info->type->validate(arg))
{
string opName = fixIdent(_name, IdentNormal);
throw RubyException(
Expand Down Expand Up @@ -434,7 +434,7 @@ IceRuby::OperationI::prepareRequest(
{
ParamInfoPtr info = *p;
volatile VALUE arg = RARRAY_AREF(args, info->pos);
if (arg != Unset && os->writeOptional(info->tag, info->type->optionalFormat()))
if (arg != Qnil && os->writeOptional(info->tag, info->type->optionalFormat()))
{
info->type->marshal(arg, os, &valueMap, true);
}
Expand Down Expand Up @@ -516,7 +516,7 @@ IceRuby::OperationI::unmarshalResults(const vector<byte>& bytes, const Ice::Comm
}
else
{
RARRAY_ASET(results, info->pos, Unset);
RARRAY_ASET(results, info->pos, Qnil);
}
}

Expand Down
2 changes: 1 addition & 1 deletion ruby/src/IceRuby/Proxy.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -805,7 +805,7 @@ IceRuby_ObjectPrx_ice_getCompress(VALUE self)
}
else
{
return Unset;
return Qnil;
}
}
ICE_RUBY_CATCH
Expand Down
76 changes: 25 additions & 51 deletions ruby/src/IceRuby/Types.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ using namespace IceRuby;
using namespace Ice;
using namespace IceInternal;

static VALUE _typeInfoClass, _exceptionInfoClass, _unsetTypeClass;
static VALUE _typeInfoClass, _exceptionInfoClass;

typedef map<string, ClassInfoPtr, std::less<>> ClassInfoMap;
static ClassInfoMap _classInfoMap;
Expand All @@ -54,8 +54,6 @@ IceRuby::resolveCompactId(int id)

namespace IceRuby
{
VALUE Unset;

class InfoMapDestroyer
{
public:
Expand Down Expand Up @@ -906,7 +904,7 @@ convertDataMembers(VALUE members, DataMemberList& reqMembers, DataMemberList& op
//
// StructInfo implementation.
//
IceRuby::StructInfo::StructInfo(VALUE ident, VALUE t, VALUE m) : rubyClass(t), _nullMarshalValue(Qnil)
IceRuby::StructInfo::StructInfo(VALUE ident, VALUE t, VALUE m) : rubyClass(t)
{
const_cast<string&>(id) = getString(ident);

Expand Down Expand Up @@ -935,7 +933,7 @@ IceRuby::StructInfo::getId() const
bool
IceRuby::StructInfo::validate(VALUE val)
{
return NIL_P(val) || callRuby(rb_obj_is_kind_of, val, rubyClass) == Qtrue;
return !NIL_P(val) && callRuby(rb_obj_is_kind_of, val, rubyClass) == Qtrue;
}

bool
Expand Down Expand Up @@ -973,17 +971,7 @@ IceRuby::StructInfo::usesClasses() const
void
IceRuby::StructInfo::marshal(VALUE p, Ice::OutputStream* os, ValueMap* valueMap, bool optional)
{
assert(NIL_P(p) || callRuby(rb_obj_is_kind_of, p, rubyClass) == Qtrue); // validate() should have caught this.

if (NIL_P(p))
{
if (NIL_P(_nullMarshalValue))
{
_nullMarshalValue = callRuby(rb_class_new_instance, 0, static_cast<VALUE*>(0), rubyClass);
rb_gc_register_address(&_nullMarshalValue); // Prevent garbage collection
}
p = _nullMarshalValue;
}
assert(!NIL_P(p) && callRuby(rb_obj_is_kind_of, p, rubyClass) == Qtrue); // validate() should have caught this.

Ice::OutputStream::size_type sizePos = 0;
if (optional)
Expand Down Expand Up @@ -1059,29 +1047,22 @@ IceRuby::StructInfo::print(VALUE value, IceInternal::Output& out, PrintObjectHis
return;
}

if (NIL_P(value))
{
out << "<nil>";
}
else
out.sb();
for (DataMemberList::const_iterator q = members.begin(); q != members.end(); ++q)
{
out.sb();
for (DataMemberList::const_iterator q = members.begin(); q != members.end(); ++q)
DataMemberPtr member = *q;
out << nl << member->name << " = ";
if (callRuby(rb_ivar_defined, value, member->rubyID) == Qfalse)
{
DataMemberPtr member = *q;
out << nl << member->name << " = ";
if (callRuby(rb_ivar_defined, value, member->rubyID) == Qfalse)
{
out << "<not defined>";
}
else
{
volatile VALUE val = callRuby(rb_ivar_get, value, member->rubyID);
member->type->print(val, out, history);
}
out << "<not defined>";
}
else
{
volatile VALUE val = callRuby(rb_ivar_get, value, member->rubyID);
member->type->print(val, out, history);
}
out.eb();
}
out.eb();
}

void
Expand All @@ -1092,11 +1073,6 @@ IceRuby::StructInfo::destroy()
(*p)->type->destroy();
}
const_cast<DataMemberList&>(members).clear();
if (!NIL_P(_nullMarshalValue))
{
rb_gc_unregister_address(&_nullMarshalValue); // Prevent garbage collection
_nullMarshalValue = Qnil;
}
}

//
Expand Down Expand Up @@ -2199,9 +2175,9 @@ IceRuby::ClassInfo::printMembers(VALUE value, IceInternal::Output& out, PrintObj
else
{
volatile VALUE val = callRuby(rb_ivar_get, value, member->rubyID);
if (val == Unset)
if (val == Qnil)
{
out << "<unset>";
out << "<nil>";
}
else
{
Expand Down Expand Up @@ -2494,7 +2470,7 @@ IceRuby::ValueWriter::writeMembers(Ice::OutputStream* os, const DataMemberList&

volatile VALUE val = callRuby(rb_ivar_get, _object, member->rubyID);

if (member->optional && (val == Unset || !os->writeOptional(member->tag, member->type->optionalFormat())))
if (member->optional && (val == Qnil || !os->writeOptional(member->tag, member->type->optionalFormat())))
{
continue;
}
Expand Down Expand Up @@ -2578,7 +2554,7 @@ IceRuby::ValueReader::_iceRead(Ice::InputStream* is)
}
else
{
callRuby(rb_ivar_set, _object, member->rubyID, Unset);
callRuby(rb_ivar_set, _object, member->rubyID, Qnil);
}
}

Expand Down Expand Up @@ -2725,7 +2701,7 @@ IceRuby::ExceptionInfo::unmarshal(Ice::InputStream* is)
}
else
{
callRuby(rb_ivar_set, obj, member->rubyID, Unset);
callRuby(rb_ivar_set, obj, member->rubyID, Qnil);
}
}

Expand Down Expand Up @@ -2791,9 +2767,9 @@ IceRuby::ExceptionInfo::printMembers(VALUE value, IceInternal::Output& out, Prin
else
{
volatile VALUE val = callRuby(rb_ivar_get, value, member->rubyID);
if (val == Unset)
if (val == Qnil)
{
out << "<unset>";
out << "<nil>";
}
else
{
Expand Down Expand Up @@ -3128,10 +3104,8 @@ IceRuby::initTypes(VALUE iceModule)
rb_define_module_function(iceModule, "__stringify", CAST_METHOD(IceRuby_stringify), 2);
rb_define_module_function(iceModule, "__stringifyException", CAST_METHOD(IceRuby_stringifyException), 1);

_unsetTypeClass = rb_define_class_under(iceModule, "Internal_UnsetType", rb_cObject);
Unset = callRuby(rb_class_new_instance, 0, static_cast<VALUE*>(0), _unsetTypeClass);
rb_undef_alloc_func(_unsetTypeClass);
rb_define_const(iceModule, "Unset", Unset);
// Keep Ice::Unset as an alias for nil, for backwards compatibility.
rb_define_const(iceModule, "Unset", Qnil);

return true;
}
Expand Down
3 changes: 0 additions & 3 deletions ruby/src/IceRuby/Types.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,6 @@ namespace IceRuby
private:
bool _variableLength;
int _wireSize;
VALUE _nullMarshalValue;
};
using StructInfoPtr = std::shared_ptr<StructInfo>;

Expand Down Expand Up @@ -524,8 +523,6 @@ namespace IceRuby
ClassInfoPtr lookupClassInfo(std::string_view);
ExceptionInfoPtr lookupExceptionInfo(std::string_view);

extern VALUE Unset;

bool initTypes(VALUE);

VALUE createType(const TypeInfoPtr&);
Expand Down
4 changes: 2 additions & 2 deletions ruby/test/Ice/operations/Twoways.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -301,9 +301,9 @@ def twoways(helper, communicator, p)
test(so.s.s == "a new string")
so.p.opVoid()

# Test marshaling of null structs and structs with null members.
# Test marshaling of structs with null members.
si1 = Test::Structure.new
si2 = nil
si2 = Test::Structure.new

rso, so = p.opStruct(si1, si2)
test(!rso.p)
Expand Down
Loading

0 comments on commit bcdd6a4

Please sign in to comment.