Skip to content

Commit

Permalink
More Optional Class Cleanup (#2262)
Browse files Browse the repository at this point in the history
  • Loading branch information
@InsertCreativityHere
InsertCreativityHere authored Jun 5, 2024
1 parent aaf3836 commit 6f87143
Show file tree
Hide file tree
Showing 27 changed files with 147 additions and 339 deletions.
3 changes: 1 addition & 2 deletions cpp/src/Ice/InputStream.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1558,8 +1558,7 @@ Ice::InputStream::skipOptional(OptionalFormat type)
}
case OptionalFormat::Class:
{
throw MarshalException{__FILE__, __LINE__, "optional class parameters and fields are no longer supported"};
break;
throw MarshalException{__FILE__, __LINE__, "cannot skip optional class"};
}
}
}
Expand Down
12 changes: 6 additions & 6 deletions cpp/src/Slice/Grammar.y
View file
Original file line number Diff line number Diff line change
Expand Up @@ -666,9 +666,9 @@ optional_type_id
m->type = ts->v.first;
m->name = ts->v.second;

// It's safe to perform this check in the parser, since even at this early stage of compilation, we have enough
// information to know whether it's okay to mark a type as optional. This is because the only types that can be
// forward declared (classes/interfaces) have constant values for `usesClasses` (true/false respectively).
// It's safe to perform this check in the parser, since we already have enough information to know whether a type
// can be optional. This is because the only types that can be forward declared (classes/interfaces) have constant
// values for `usesClasses` (true/false respectively).
if (m->type->usesClasses())
{
currentUnit->error("types that use classes cannot be marked with 'optional'");
Expand Down Expand Up @@ -1136,9 +1136,9 @@ return_type
auto m = dynamic_pointer_cast<OptionalDefTok>($1);
m->type = dynamic_pointer_cast<Type>($2);

// It's safe to perform this check in the parser, since even at this early stage of compilation, we have enough
// information to know whether it's okay to mark a type as optional. This is because the only types that can be
// forward declared (classes/interfaces) have constant values for `usesClasses` (true/false respectively).
// It's safe to perform this check in the parser, since we already have enough information to know whether a type
// can be optional. This is because the only types that can be forward declared (classes/interfaces) have constant
// values for `usesClasses` (true/false respectively).
if (m->type->usesClasses())
{
currentUnit->error("types that use classes cannot be marked with 'optional'");
Expand Down
17 changes: 13 additions & 4 deletions cpp/test/Ice/optional/AllTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,9 @@ class DObjectWriter : public Ice::Value
o->push_back("test3");
o->push_back("test4");
out->write(1, o);
APtr a = make_shared<A>();
a->mc = 18;
out->write(a);
out->endSlice();
// ::Test::B
out->startSlice(B::ice_staticId(), -1, false);
Expand Down Expand Up @@ -164,6 +167,7 @@ class DObjectReader : public Ice::Value
test(
o && o->size() == 4 && (*o)[0] == "test1" && (*o)[1] == "test2" && (*o)[2] == "test3" &&
(*o)[3] == "test4");
in->read(a);
in->endSlice();
// ::Test::B
in->startSlice();
Expand All @@ -177,12 +181,17 @@ class DObjectReader : public Ice::Value
in->endValue();
}

void check() { test(a->mc == 18); }

protected:
virtual Ice::ValuePtr _iceCloneImpl() const
{
assert(0); // not used
return nullptr;
}

private:
APtr a;
};

class FObjectReader : public Ice::Value
Expand Down Expand Up @@ -770,13 +779,12 @@ allTests(Test::TestHelper* helper, bool)
{
cout << "testing marshaling with unknown class slices... " << flush;
{
CPtr c = make_shared<C>();
c->ss = "test";
c->ms = string("testms");

{
Ice::OutputStream out(communicator);
out.startEncapsulation();
CPtr c = make_shared<C>();
c->ss = "test";
c->ms = string("testms");
out.write(c);
out.endEncapsulation();
out.finished(inEncaps);
Expand Down Expand Up @@ -806,6 +814,7 @@ allTests(Test::TestHelper* helper, bool)
in.read(obj);
in.endEncapsulation();
test(obj && dynamic_cast<DObjectReader*>(obj.get()));
dynamic_cast<DObjectReader*>(obj.get())->check();
factory->setEnabled(false);
}
}
Expand Down
20 changes: 6 additions & 14 deletions csharp/src/Ice/InputStream.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2595,7 +2595,7 @@ private void skipOptional(OptionalFormat format)
}
case OptionalFormat.Class:
{
throw new MarshalException("cannot unmarshal or skip an \"optional class\" parameter or field");
throw new MarshalException("cannot skip an optional class");
}
}
}
Expand Down Expand Up @@ -2911,8 +2911,6 @@ internal EncapsDecoder10(InputStream stream, Encaps encaps, bool sliceValues, in

internal override void readValue(System.Action<Value?> cb)
{
Debug.Assert(cb != null);

//
// Object references are encoded as a negative integer in 1.0.
//
Expand Down Expand Up @@ -3241,10 +3239,7 @@ internal override void readValue(System.Action<Value?> cb)
}
else if (index == 0)
{
if (cb != null)
{
cb(null);
}
cb(null);
}
else if (_current != null && (_current.sliceFlags & Protocol.FLAG_HAS_INDIRECTION_TABLE) != 0)
{
Expand All @@ -3259,15 +3254,12 @@ internal override void readValue(System.Action<Value?> cb)
// derive an index into the indirection table that we'll read
// at the end of the slice.
//
if (cb != null)
if (_current.indirectPatchList == null)
{
if (_current.indirectPatchList == null)
{
_current.indirectPatchList = new Stack<IndirectPatchEntry>();
}
IndirectPatchEntry e = new IndirectPatchEntry(index - 1, cb);
_current.indirectPatchList.Push(e);
_current.indirectPatchList = new Stack<IndirectPatchEntry>();
}
IndirectPatchEntry e = new IndirectPatchEntry(index - 1, cb);
_current.indirectPatchList.Push(e);
}
else
{
Expand Down
26 changes: 24 additions & 2 deletions csharp/test/Ice/optional/AllTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1692,6 +1692,30 @@ public class AllTests : global::Test.AllTests
m = Test.StringIntDictHelper.read(@in);
test(MapsEqual(m, p1));
@in.endEncapsulation();

@in = new InputStream(communicator, outEncaps);
@in.startEncapsulation();
@in.endEncapsulation();

Test.F f = new Test.F();
f.fsf = new Test.FixedStruct(56);
f.fse = f.fsf.Value;

os = new OutputStream(communicator);
os.startEncapsulation();
os.writeOptional(2, OptionalFormat.VSize);
os.writeSize(4);
Test.FixedStruct.ice_write(os, f.fse);
os.endEncapsulation();
inEncaps = os.finished();

@in = new InputStream(communicator, inEncaps);
@in.startEncapsulation();
test(@in.readOptional(2, OptionalFormat.VSize));
@in.skipSize();
Test.FixedStruct fs1 = Test.FixedStruct.ice_read(@in);
@in.endEncapsulation();
test(fs1.m == 56);
}
output.WriteLine("ok");

Expand Down Expand Up @@ -2000,7 +2024,6 @@ public override void write(Ice.OutputStream @out)
@out.endSize(pos);
Test.A a = new Test.A();
a.mc = 18;
@out.writeOptional(1000, Ice.OptionalFormat.Class);
@out.writeValue(a);
@out.endSlice();
// ::Test::B
Expand Down Expand Up @@ -2030,7 +2053,6 @@ public override void read(Ice.InputStream @in)
string[] o = @in.readStringSeq();
test(o.Length == 4 &&
o[0] == "test1" && o[1] == "test2" && o[2] == "test3" && o[3] == "test4");
test(@in.readOptional(1000, Ice.OptionalFormat.Class));
@in.readValue(a.invoke);
@in.endSlice();
// ::Test::B
Expand Down
82 changes: 17 additions & 65 deletions java/src/Ice/src/main/java/com/zeroc/Ice/InputStream.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1597,18 +1597,14 @@ public int readEnum(int maxValue) {
*/
public <T extends Value> void readValue(java.util.function.Consumer<T> cb, Class<T> cls) {
initEncaps();
if (cb == null) {
_encapsStack.decoder.readValue(null);
} else {
_encapsStack.decoder.readValue(
v -> {
if (v == null || cls.isInstance(v)) {
cb.accept(cls.cast(v));
} else {
com.zeroc.IceInternal.Ex.throwUOE(cls, v);
}
});
}
_encapsStack.decoder.readValue(
v -> {
if (v == null || cls.isInstance(v)) {
cb.accept(cls.cast(v));
} else {
com.zeroc.IceInternal.Ex.throwUOE(cls, v);
}
});
}

/**
Expand All @@ -1622,43 +1618,6 @@ public void readValue(java.util.function.Consumer<Value> cb) {
readValue(cb, Value.class);
}

/**
* Extracts an optional Slice value from the stream.
*
* @param <T> The value type.
* @param tag The numeric tag associated with the value.
* @param cb The consumer to notify when the extracted instance is available. The stream extracts
* Slice values in stages. The Ice run time calls accept on the consumer when the
* corresponding instance has been fully unmarshaled.
* @param cls The type of the Ice.Value to unmarshal.
*/
public <T extends Value> void readValue(
int tag, java.util.function.Consumer<java.util.Optional<T>> cb, Class<T> cls) {
if (readOptional(tag, OptionalFormat.Class)) {
if (cb != null) {
readValue(v -> cb.accept(java.util.Optional.ofNullable(v)), cls);
} else {
readValue(null);
}
} else {
if (cb != null) {
cb.accept(java.util.Optional.empty());
}
}
}

/**
* Extracts an optional Slice value from the stream.
*
* @param tag The numeric tag associated with the value.
* @param cb The consumer to notify when the extracted instance is available. The stream extracts
* Slice values in stages. The Ice run time calls accept on the consumer when the
* corresponding instance has been fully unmarshaled.
*/
public void readValue(int tag, java.util.function.Consumer<java.util.Optional<Value>> cb) {
readValue(tag, cb, Value.class);
}

/**
* Extracts a user exception from the stream and throws it.
*
Expand Down Expand Up @@ -1758,8 +1717,7 @@ private void skipOptional(OptionalFormat format) {
}
case Class:
{
readValue(null, null);
break;
throw new MarshalException("cannot skip an optional class");
}
}
}
Expand Down Expand Up @@ -2130,8 +2088,6 @@ private static final class EncapsDecoder10 extends EncapsDecoder {

@Override
void readValue(java.util.function.Consumer<Value> cb) {
assert (cb != null);

//
// Object references are encoded as a negative integer in 1.0.
//
Expand Down Expand Up @@ -2392,9 +2348,7 @@ void readValue(java.util.function.Consumer<Value> cb) {
if (index < 0) {
throw new MarshalException("invalid object id");
} else if (index == 0) {
if (cb != null) {
cb.accept(null);
}
cb.accept(null);
} else if (_current != null
&& (_current.sliceFlags & Protocol.FLAG_HAS_INDIRECTION_TABLE) != 0) {
//
Expand All @@ -2408,16 +2362,14 @@ void readValue(java.util.function.Consumer<Value> cb) {
// derive an index into the indirection table that we'll read
// at the end of the slice.
//
if (cb != null) {
if (_current.indirectPatchList == null) // Lazy initialization
{
_current.indirectPatchList = new java.util.ArrayDeque<>();
}
IndirectPatchEntry e = new IndirectPatchEntry();
e.index = index - 1;
e.cb = cb;
_current.indirectPatchList.push(e);
if (_current.indirectPatchList == null) // Lazy initialization
{
_current.indirectPatchList = new java.util.ArrayDeque<>();
}
IndirectPatchEntry e = new IndirectPatchEntry();
e.index = index - 1;
e.cb = cb;
_current.indirectPatchList.push(e);
} else {
readInstance(index, cb);
}
Expand Down
25 changes: 0 additions & 25 deletions java/src/Ice/src/main/java/com/zeroc/Ice/OutputStream.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1452,31 +1452,6 @@ public void writeValue(Value v) {
_encapsStack.encoder.writeValue(v);
}

/**
* Writes an optional value to the stream.
*
* @param tag The optional tag.
* @param v The optional value to write to the stream.
* @param <T> The type of the optional value.
*/
public <T extends Value> void writeValue(int tag, java.util.Optional<T> v) {
if (v != null && v.isPresent()) {
writeValue(tag, v.get());
}
}

/**
* Writes an optional value to the stream.
*
* @param tag The optional tag.
* @param v The value to write to the stream.
*/
public void writeValue(int tag, Value v) {
if (writeOptional(tag, OptionalFormat.Class)) {
writeValue(v);
}
}

/**
* Writes a user exception to the stream.
*
Expand Down
Loading

0 comments on commit 6f87143

Please sign in to comment.