Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update optional mapping in Ruby #2566

Merged
merged 3 commits into from
Jul 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions ruby/src/IceRuby/Operation.cpp
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
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
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
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
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
Loading