From kde-commits Mon Oct 17 23:32:34 2016 From: Andreas Hartmetz Date: Mon, 17 Oct 2016 23:32:34 +0000 To: kde-commits Subject: [dferry] /: Arguments::Reader/Writer: add isInsideEmptyArray() and currentSignature(). Message-Id: X-MARC-Message: https://marc.info/?l=kde-commits&m=147674716423023 Git commit e5ebd40e0ebf7123d6106057b332ed29d38c5509 by Andreas Hartmetz. Committed on 17/10/2016 at 23:22. Pushed by ahartmetz into branch 'master'. Arguments::Reader/Writer: add isInsideEmptyArray() and currentSignature(). They will (probably) be needed in QtDBus and they are somewhat useful in any case. M +21 -22 applications/analyzer/argumentsmodel.cpp M +83 -26 serialization/arguments.cpp M +4 -0 serialization/arguments.h M +8 -15 tests/serialization/tst_arguments.cpp http://commits.kde.org/dferry/e5ebd40e0ebf7123d6106057b332ed29d38c5509 diff --git a/applications/analyzer/argumentsmodel.cpp b/applications/analyz= er/argumentsmodel.cpp index 0dc97b6..4e8dbb4 100644 --- a/applications/analyzer/argumentsmodel.cpp +++ b/applications/analyzer/argumentsmodel.cpp @@ -88,7 +88,8 @@ QAbstractItemModel* createArgumentsModel(Message *message) } = bool isDone =3D false; - int emptyNesting =3D 0; + // Cache it, don't call Reader::isInsideEmptyArray() on every data ele= ment. + bool inEmptyArray =3D false; = while (!isDone) { switch(reader.state()) { @@ -112,63 +113,61 @@ QAbstractItemModel* createArgumentsModel(Message *mes= sage) parent =3D ascend(parent, model); break; case Arguments::BeginArray: { - const bool hasData =3D reader.beginArray(Arguments::Reader::Re= adTypesOnlyIfEmpty); - parent =3D descend(parent, hasData ? "Array" : "Array (no elem= ents, showing just types)"); - emptyNesting +=3D hasData ? 0 : 1; + inEmptyArray =3D !reader.beginArray(Arguments::Reader::ReadTyp= esOnlyIfEmpty); + parent =3D descend(parent, inEmptyArray ? "Array (no elements,= showing just types)" : "Array"); break; } case Arguments::EndArray: reader.endArray(); + inEmptyArray =3D reader.isInsideEmptyArray(); parent =3D ascend(parent, model); - emptyNesting =3D qMax(emptyNesting - 1, 0); break; case Arguments::BeginDict: { - const bool hasData =3D reader.beginDict(Arguments::Reader::Rea= dTypesOnlyIfEmpty); - parent =3D descend(parent, hasData ? "Dict" : "Dict (no elemen= ts, showing just types)"); - emptyNesting +=3D hasData ? 0 : 1; + inEmptyArray =3D !reader.beginDict(Arguments::Reader::ReadType= sOnlyIfEmpty); + parent =3D descend(parent, inEmptyArray ? "Dict (no elements, = showing just types)" : "Dict"); break; } case Arguments::EndDict: reader.endDict(); + inEmptyArray =3D reader.isInsideEmptyArray(); parent =3D ascend(parent, model); - emptyNesting =3D qMax(emptyNesting - 1, 0); break; case Arguments::Byte: - addKeyValue(parent, "byte", emptyNesting, reader.readByte()); + addKeyValue(parent, "byte", inEmptyArray, reader.readByte()); break; case Arguments::Boolean: - addKeyValue(parent, "boolean", emptyNesting, reader.readBoolea= n()); + addKeyValue(parent, "boolean", inEmptyArray, reader.readBoolea= n()); break; case Arguments::Int16: - addKeyValue(parent, "int16", emptyNesting, reader.readInt16()); + addKeyValue(parent, "int16", inEmptyArray, reader.readInt16()); break; case Arguments::Uint16: - addKeyValue(parent, "uint16", emptyNesting, reader.readUint16(= )); + addKeyValue(parent, "uint16", inEmptyArray, reader.readUint16(= )); break; case Arguments::Int32: - addKeyValue(parent, "int32", emptyNesting, reader.readInt32()); + addKeyValue(parent, "int32", inEmptyArray, reader.readInt32()); break; case Arguments::Uint32: - addKeyValue(parent, "uint32", emptyNesting, reader.readUint32(= )); + addKeyValue(parent, "uint32", inEmptyArray, reader.readUint32(= )); break; case Arguments::Int64: - addKeyValue(parent, "int64", emptyNesting, reader.readInt64()); + addKeyValue(parent, "int64", inEmptyArray, reader.readInt64()); break; case Arguments::Uint64: - addKeyValue(parent, "uint64", emptyNesting, reader.readUint64(= )); + addKeyValue(parent, "uint64", inEmptyArray, reader.readUint64(= )); break; case Arguments::Double: - addKeyValue(parent, "double", emptyNesting, reader.readDouble(= )); + addKeyValue(parent, "double", inEmptyArray, reader.readDouble(= )); break; case Arguments::String: - addKeyValue(parent, "string", emptyNesting, reader.readString(= ).ptr); + addKeyValue(parent, "string", inEmptyArray, reader.readString(= ).ptr); break; case Arguments::ObjectPath: - addKeyValue(parent, "object path", emptyNesting, reader.readOb= jectPath().ptr); + addKeyValue(parent, "object path", inEmptyArray, reader.readOb= jectPath().ptr); break; case Arguments::Signature: - addKeyValue(parent, "type signature", emptyNesting, reader.rea= dSignature().ptr); + addKeyValue(parent, "type signature", inEmptyArray, reader.rea= dSignature().ptr); break; case Arguments::UnixFd: - addKeyValue(parent, "file descriptor", emptyNesting, QVariant(= )); + addKeyValue(parent, "file descriptor", inEmptyArray, QVariant(= )); break; case Arguments::InvalidData: case Arguments::NeedMoreData: diff --git a/serialization/arguments.cpp b/serialization/arguments.cpp index 0e7c5a5..6c03f83 100644 --- a/serialization/arguments.cpp +++ b/serialization/arguments.cpp @@ -507,7 +507,10 @@ std::string Arguments::prettyPrint() const std::string nestingPrefix; = bool isDone =3D false; - int emptyNesting =3D 0; + + // Cache it, don't call Reader::isInsideEmptyArray() on every data ele= ment. This isn't really + // a big deal for performance here, but in other situations it is, so = set a good example :) + bool inEmptyArray =3D false; = while (!isDone) { // HACK use nestingPrefix to determine when we're switching from k= ey to value - this can be done @@ -548,27 +551,26 @@ std::string Arguments::prettyPrint() const const std::pair bytes =3D reade= r.readPrimitiveArray(); assert(bytes.first =3D=3D Arguments::Byte); assert(bytes.second.length > 0); + inEmptyArray =3D reader.isInsideEmptyArray(); // Maybe not= necessary, but safe ret << nestingPrefix << "array of bytes [ " << uint(bytes.= second.ptr[0]); for (uint32 i =3D 1; i < bytes.second.length; i++) { ret << ", " << uint(bytes.second.ptr[i]); } ret << " ]\n"; } else { - const bool hasData =3D reader.beginArray(Arguments::Reader= ::ReadTypesOnlyIfEmpty); - emptyNesting +=3D hasData ? 0 : 1; + inEmptyArray =3D !reader.beginArray(Arguments::Reader::Rea= dTypesOnlyIfEmpty); ret << nestingPrefix << "begin array\n"; nestingPrefix +=3D "[ "; } break; case Arguments::EndArray: reader.endArray(); - emptyNesting =3D std::max(emptyNesting - 1, 0); + inEmptyArray =3D reader.isInsideEmptyArray(); nestingPrefix.resize(nestingPrefix.size() - 2); ret << nestingPrefix << "end array\n"; break; case Arguments::BeginDict: { - const bool hasData =3D reader.beginDict(Arguments::Reader::Rea= dTypesOnlyIfEmpty); - emptyNesting +=3D hasData ? 0 : 1; + inEmptyArray =3D !reader.beginDict(Arguments::Reader::ReadType= sOnlyIfEmpty); ret << nestingPrefix << "begin dict\n"; nestingPrefix +=3D "{K "; break; } @@ -583,14 +585,14 @@ std::string Arguments::prettyPrint() const #endif case Arguments::EndDict: reader.endDict(); - emptyNesting =3D std::max(emptyNesting - 1, 0); + inEmptyArray =3D reader.isInsideEmptyArray(); nestingPrefix.resize(nestingPrefix.size() - strlen("{V ")); ret << nestingPrefix << "end dict\n"; break; case Arguments::Boolean: { bool b =3D reader.readBoolean(); ret << nestingPrefix << "bool: "; - if (emptyNesting) { + if (inEmptyArray) { ret << ""; } else { ret << (b ? "true" : "false"); @@ -598,37 +600,37 @@ std::string Arguments::prettyPrint() const ret << '\n'; break; } case Arguments::Byte: - printMaybeNil(&ret, nestingPrefix, emptyNesting, int(reader.re= adByte()), "byte"); + printMaybeNil(&ret, nestingPrefix, inEmptyArray, int(reader.re= adByte()), "byte"); break; case Arguments::Int16: - printMaybeNil(&ret, nestingPrefix, emptyNesting, reader.readIn= t16(), "int16"); + printMaybeNil(&ret, nestingPrefix, inEmptyArray, reader.readIn= t16(), "int16"); break; case Arguments::Uint16: - printMaybeNil(&ret, nestingPrefix, emptyNesting, reader.readUi= nt16(), "uint16"); + printMaybeNil(&ret, nestingPrefix, inEmptyArray, reader.readUi= nt16(), "uint16"); break; case Arguments::Int32: - printMaybeNil(&ret, nestingPrefix, emptyNesting, reader.readIn= t32(), "int32"); + printMaybeNil(&ret, nestingPrefix, inEmptyArray, reader.readIn= t32(), "int32"); break; case Arguments::Uint32: - printMaybeNil(&ret, nestingPrefix, emptyNesting, reader.readUi= nt32(), "uint32"); + printMaybeNil(&ret, nestingPrefix, inEmptyArray, reader.readUi= nt32(), "uint32"); break; case Arguments::Int64: - printMaybeNil(&ret, nestingPrefix, emptyNesting, reader.readIn= t64(), "int64"); + printMaybeNil(&ret, nestingPrefix, inEmptyArray, reader.readIn= t64(), "int64"); break; case Arguments::Uint64: - printMaybeNil(&ret, nestingPrefix, emptyNesting, reader.readUi= nt64(), "uint64"); + printMaybeNil(&ret, nestingPrefix, inEmptyArray, reader.readUi= nt64(), "uint64"); break; case Arguments::Double: - printMaybeNil(&ret, nestingPrefix, emptyNesting, reader.readDo= uble(), "double"); + printMaybeNil(&ret, nestingPrefix, inEmptyArray, reader.readDo= uble(), "double"); break; case Arguments::String: - printMaybeNil(&ret, nestingPrefix, emptyNesting, reader.readSt= ring(), "string"); + printMaybeNil(&ret, nestingPrefix, inEmptyArray, reader.readSt= ring(), "string"); break; case Arguments::ObjectPath: - printMaybeNil(&ret, nestingPrefix, emptyNesting, reader.readOb= jectPath(), "object path"); + printMaybeNil(&ret, nestingPrefix, inEmptyArray, reader.readOb= jectPath(), "object path"); break; case Arguments::Signature: - printMaybeNil(&ret, nestingPrefix, emptyNesting, reader.readSi= gnature(), "type signature"); + printMaybeNil(&ret, nestingPrefix, inEmptyArray, reader.readSi= gnature(), "type signature"); break; case Arguments::UnixFd: // TODO @@ -940,6 +942,36 @@ cstring Arguments::Reader::stateString() const return printableState(m_state); } = +bool Arguments::Reader::isInsideEmptyArray() const +{ + if (d->m_nilArrayNesting > 1) { + return true; + } else if (d->m_nilArrayNesting =3D=3D 1) { + // Special case 1: in state BeginArray/Dict, when we've just enter= ed the nil array according + // to our internal state, the API user cannot know that yet -> ret= urn false; + // Since this is easily done here, don't complicate the rest of th= e code with a hacky solution. + if (m_state =3D=3D BeginArray || m_state =3D=3D BeginDict) { + return false; + } + // Special case 2: in state EndArray/Dict, when we've just left th= e nil array according + // to our internal state, advanceState() and endArray/Dict() make = sure that + // d->m_nilArrayNesting is only decremented after endArray/Dict() = has been called. + // Nothing to do for us here, but this is the point to explain it. + return true; + } else { + assert(d->m_nilArrayNesting =3D=3D 0); + return false; + } +} + +cstring Arguments::Reader::currentSignature() const +{ + // ### not sure if determining length like that is the best way, shoul= d probably be as similar to + // libdbus-1 as possible (unless we can do much better). + return cstring(d->m_signature.ptr, + std::max(uint32(0), std::min(d->m_signature.length, d->= m_signaturePosition))); +} + void Arguments::Reader::replaceData(chunk data) { VALID_IF(data.length >=3D d->m_dataPosition, Error::ReplacementDataIsS= horter); @@ -1167,20 +1199,24 @@ void Arguments::Reader::advanceState() const bool isDict =3D aggregateInfo.aggregateType =3D=3D Begin= Dict; if (d->m_signaturePosition > aggregateInfo.arr.containedTypeBe= gin + (isDict ? 1 : 0)) { const Private::ArrayInfo &arrayInfo =3D aggregateInfo.arr; - bool moreElements =3D false; + bool moreIterations =3D false; if (unlikely(d->m_nilArrayNesting)) { // we're done iterating over it once // TODO unit-test this - d->m_nilArrayNesting--; + // HACK: decreasing m_nilArrayNesting here would be mu= ch cleaner! But from the + // user's POV, the nil array has not been left before = the call to endArray(). + // We will set state EndArray / EndDict, so we know th= at the user "must" call + // endArray()/endDict(), so adjust d->m_nilArrayNestin= g in those methods. + //d->m_nilArrayNesting--; } else if (d->m_dataPosition < arrayInfo.dataEnd) { if (isDict) { d->m_dataPosition =3D align(d->m_dataPosition, 8);= // align to dict entry } // rewind to start of contained type and read the type= info there d->m_signaturePosition =3D arrayInfo.containedTypeBegi= n; - moreElements =3D true; + moreIterations =3D true; } - if (!moreElements) { + if (!moreIterations) { // TODO check that final data position is where it sho= uld be according to the // serialized array length m_state =3D isDict ? EndDict : EndArray; @@ -1326,7 +1362,8 @@ void Arguments::Reader::advanceState() VALID_IF(d->m_nesting.beginArray(), Error::MalformedMessageData); if (firstElementTy.state() =3D=3D BeginDict) { d->m_signaturePosition++; - // TODO check whether the first type is a primitive or string = type! + // TODO check whether the first type is a primitive or string = type! // ### isn't that already + // checked for the main signature and / or variants, though? // only closed at end of dict - there is no observable differe= nce for clients VALID_IF(d->m_nesting.beginParen(), Error::MalformedMessageDat= a); } @@ -1462,7 +1499,11 @@ void Arguments::Reader::skipArray() = void Arguments::Reader::endArray() { - advanceStateFrom(EndArray); + VALID_IF(m_state =3D=3D EndArray, Error::ReadWrongType); + if (unlikely(d->m_nilArrayNesting)) { + d->m_nilArrayNesting--; + } + advanceState(); } = static bool isAligned(uint32 value, uint32 alignment) @@ -1562,7 +1603,11 @@ void Arguments::Reader::skipDict() = void Arguments::Reader::endDict() { - advanceStateFrom(EndDict); + VALID_IF(m_state =3D=3D EndDict, Error::ReadWrongType); + if (unlikely(d->m_nilArrayNesting)) { + d->m_nilArrayNesting--; + } + advanceState(); } = void Arguments::Reader::beginStruct() @@ -1853,6 +1898,18 @@ cstring Arguments::Writer::stateString() const return printableState(m_state); } = +bool Arguments::Writer::isInsideEmptyArray() const +{ + return d->m_nilArrayNesting > 0; +} + +cstring Arguments::Writer::currentSignature() const +{ + // ### see comment in Reader::currentSignature + return cstring(d->m_signature.ptr, + std::max(uint32(0), std::min(d->m_signature.length, d->= m_signaturePosition))); +} + void Arguments::Writer::doWritePrimitiveType(uint32 alignAndSize) { d->m_dataPosition =3D align(d->m_dataPosition, alignAndSize); diff --git a/serialization/arguments.h b/serialization/arguments.h index d557c8a..cf7719d 100644 --- a/serialization/arguments.h +++ b/serialization/arguments.h @@ -169,6 +169,8 @@ public: = IoState state() const { return m_state; } cstring stateString() const; + bool isInsideEmptyArray() const; + cstring currentSignature() const; // current signature, either mai= n signature or current variant // HACK call this in NeedMoreData state when more data has been ad= ded; this replaces m_data // WARNING: calling replaceData() invalidates copies (if any) of t= his Reader void replaceData(chunk data); @@ -288,6 +290,8 @@ public: = IoState state() const { return m_state; } cstring stateString() const; + bool isInsideEmptyArray() const; + cstring currentSignature() const; // current signature, either mai= n signature or current variant = enum ArrayOption { diff --git a/tests/serialization/tst_arguments.cpp b/tests/serialization/ts= t_arguments.cpp index 3546d04..39dce0b 100644 --- a/tests/serialization/tst_arguments.cpp +++ b/tests/serialization/tst_arguments.cpp @@ -70,7 +70,7 @@ static bool stringsEqual(cstring s1, cstring s2) // 3) skips nil arrays at and below nil array nesting level m_skipNilArray= sFromLevel with m_skippingReader // It even skips aggregates inside nil arrays as 2) + 3) imply. // It checks: -// a) where nothing is skipped that the aggregate structure and data read = is the same +// a) where nothing is skipped, that the aggregate structure and data read= are the same class SkipChecker { public: @@ -284,7 +284,7 @@ static void testReadWithSkip(const Arguments &arg, bool= debugPrint) = // When using this to iterate over the reader, it will make an exact copy = using the Writer. // You need to do something only in states where something special should = happen. -static void defaultReadToWrite(Arguments::Reader *reader, Arguments::Write= r *writer, uint32 *emptyNesting) +static void defaultReadToWrite(Arguments::Reader *reader, Arguments::Write= r *writer) { switch(reader->state()) { case Arguments::BeginStruct: @@ -307,23 +307,19 @@ static void defaultReadToWrite(Arguments::Reader *rea= der, Arguments::Writer *wri const bool hasData =3D reader->beginArray(Arguments::Reader::ReadT= ypesOnlyIfEmpty); writer->beginArray(hasData ? Arguments::Writer::NonEmptyArray : Arguments::Writer::WriteTypesOfEmpty= Array); - *emptyNesting +=3D hasData ? 0 : 1; break; } case Arguments::EndArray: reader->endArray(); writer->endArray(); - *emptyNesting =3D std::max(*emptyNesting - 1, 0u); break; case Arguments::BeginDict: { const bool hasData =3D reader->beginDict(Arguments::Reader::ReadTy= pesOnlyIfEmpty); writer->beginDict(hasData ? Arguments::Writer::NonEmptyArray : Arguments::Writer::WriteTypesOfEmpty= Array); - *emptyNesting +=3D hasData ? 0 : 1; break; } case Arguments::EndDict: reader->endDict(); writer->endDict(); - *emptyNesting =3D std::max(*emptyNesting - 1, 0u); break; case Arguments::Byte: writer->writeByte(reader->readByte()); @@ -354,7 +350,7 @@ static void defaultReadToWrite(Arguments::Reader *reade= r, Arguments::Writer *wri break; case Arguments::String: { cstring s =3D reader->readString(); - if (*emptyNesting) { + if (reader->isInsideEmptyArray()) { s =3D cstring(""); } else { TEST(Arguments::isStringValid(s)); @@ -363,7 +359,7 @@ static void defaultReadToWrite(Arguments::Reader *reade= r, Arguments::Writer *wri break; } case Arguments::ObjectPath: { cstring objectPath =3D reader->readObjectPath(); - if (*emptyNesting) { + if (reader->isInsideEmptyArray()) { objectPath =3D cstring("/"); } else { TEST(Arguments::isObjectPathValid(objectPath)); @@ -372,7 +368,7 @@ static void defaultReadToWrite(Arguments::Reader *reade= r, Arguments::Writer *wri break; } case Arguments::Signature: { cstring signature =3D reader->readSignature(); - if (*emptyNesting) { + if (reader->isInsideEmptyArray()) { signature =3D cstring(""); } else { TEST(Arguments::isSignatureValid(signature)); @@ -428,7 +424,6 @@ static void doRoundtripWithShortReads(const Arguments &= original, uint32 dataIncr chunk data =3D original.data(); chunk shortData; bool isDone =3D false; - uint32 emptyNesting =3D 0; = while (!isDone) { TEST(writer.state() !=3D Arguments::InvalidData); @@ -460,7 +455,7 @@ static void doRoundtripWithShortReads(const Arguments &= original, uint32 dataIncr reader.replaceData(shortData); break; } default: - defaultReadToWrite(&reader, &writer, &emptyNesting); + defaultReadToWrite(&reader, &writer); break; } } @@ -478,7 +473,6 @@ static void doRoundtripWithReaderCopy(const Arguments &= original, uint32 dataIncr Arguments::Writer writer; = bool isDone =3D false; - uint32 emptyNesting =3D 0; uint32 i =3D 0; = while (!isDone) { @@ -496,7 +490,7 @@ static void doRoundtripWithReaderCopy(const Arguments &= original, uint32 dataIncr isDone =3D true; break; default: - defaultReadToWrite(reader, &writer, &emptyNesting); + defaultReadToWrite(reader, &writer); break; } } @@ -512,7 +506,6 @@ static void doRoundtripWithWriterCopy(const Arguments &= original, uint32 dataIncr Arguments::Writer *writer =3D new Arguments::Writer; = bool isDone =3D false; - uint32 emptyNesting =3D 0; uint32 i =3D 0; = while (!isDone) { @@ -530,7 +523,7 @@ static void doRoundtripWithWriterCopy(const Arguments &= original, uint32 dataIncr isDone =3D true; break; default: - defaultReadToWrite(&reader, writer, &emptyNesting); + defaultReadToWrite(&reader, writer); break; } }