Commit b2edb433 authored by Kenton Varda's avatar Kenton Varda

More dynamic API WIP.

parent 18cfa132
...@@ -45,6 +45,9 @@ class ReaderArena; ...@@ -45,6 +45,9 @@ class ReaderArena;
class BuilderArena; class BuilderArena;
class ReadLimiter; class ReadLimiter;
class Segment;
typedef Id<uint32_t, Segment> SegmentId;
class ReadLimiter { class ReadLimiter {
// Used to keep track of how much data has been processed from a message, and cut off further // Used to keep track of how much data has been processed from a message, and cut off further
// processing if and when a particular limit is reached. This is primarily intended to guard // processing if and when a particular limit is reached. This is primarily intended to guard
......
This diff is collapsed.
This diff is collapsed.
...@@ -31,6 +31,8 @@ ...@@ -31,6 +31,8 @@
namespace capnproto { namespace capnproto {
class SchemaPool; // Needs to be declared for dynamic Object accessors.
class DynamicStruct; // So that it can be declared a friend. class DynamicStruct; // So that it can be declared a friend.
template <typename T> template <typename T>
...@@ -52,7 +54,7 @@ struct PointerHelpers<T, Kind::STRUCT> { ...@@ -52,7 +54,7 @@ struct PointerHelpers<T, Kind::STRUCT> {
} }
static inline void set(StructBuilder builder, WireReferenceCount index, static inline void set(StructBuilder builder, WireReferenceCount index,
typename T::Reader value) { typename T::Reader value) {
// TODO(soon) // TODO(now): schemaless copy
CAPNPROTO_INLINE_PRECOND(false, "Not implemented: set() for struct fields."); CAPNPROTO_INLINE_PRECOND(false, "Not implemented: set() for struct fields.");
} }
static inline typename T::Builder init(StructBuilder builder, WireReferenceCount index) { static inline typename T::Builder init(StructBuilder builder, WireReferenceCount index) {
......
...@@ -138,7 +138,7 @@ static void setupStruct(StructBuilder builder) { ...@@ -138,7 +138,7 @@ static void setupStruct(StructBuilder builder) {
2 * REFERENCES, 4 * ELEMENTS, STRUCTLIST_ELEMENT_SIZE); 2 * REFERENCES, 4 * ELEMENTS, STRUCTLIST_ELEMENT_SIZE);
EXPECT_EQ(4 * ELEMENTS, list.size()); EXPECT_EQ(4 * ELEMENTS, list.size());
for (int i = 0; i < 4; i++) { for (int i = 0; i < 4; i++) {
StructBuilder element = list.getStructElement(i * ELEMENTS, STRUCTLIST_ELEMENT_SIZE); StructBuilder element = list.getStructElement(i * ELEMENTS);
element.setDataField<int32_t>(0 * ELEMENTS, 300 + i); element.setDataField<int32_t>(0 * ELEMENTS, 300 + i);
element.initStructField(0 * REFERENCES, element.initStructField(0 * REFERENCES,
StructSize(1 * WORDS, 0 * REFERENCES, FieldSize::EIGHT_BYTES)) StructSize(1 * WORDS, 0 * REFERENCES, FieldSize::EIGHT_BYTES))
...@@ -193,7 +193,7 @@ static void checkStruct(StructBuilder builder) { ...@@ -193,7 +193,7 @@ static void checkStruct(StructBuilder builder) {
ListBuilder list = builder.getListField(2 * REFERENCES, nullptr); ListBuilder list = builder.getListField(2 * REFERENCES, nullptr);
ASSERT_EQ(4 * ELEMENTS, list.size()); ASSERT_EQ(4 * ELEMENTS, list.size());
for (int i = 0; i < 4; i++) { for (int i = 0; i < 4; i++) {
StructBuilder element = list.getStructElement(i * ELEMENTS, STRUCTLIST_ELEMENT_SIZE); StructBuilder element = list.getStructElement(i * ELEMENTS);
EXPECT_EQ(300 + i, element.getDataField<int32_t>(0 * ELEMENTS)); EXPECT_EQ(300 + i, element.getDataField<int32_t>(0 * ELEMENTS));
EXPECT_EQ(400 + i, EXPECT_EQ(400 + i,
element.getStructField(0 * REFERENCES, element.getStructField(0 * REFERENCES,
......
...@@ -1250,11 +1250,11 @@ Data::Builder ListBuilder::asData() { ...@@ -1250,11 +1250,11 @@ Data::Builder ListBuilder::asData() {
return Data::Builder(reinterpret_cast<char*>(ptr), elementCount / ELEMENTS); return Data::Builder(reinterpret_cast<char*>(ptr), elementCount / ELEMENTS);
} }
StructBuilder ListBuilder::getStructElement(ElementCount index, StructSize elementSize) const { StructBuilder ListBuilder::getStructElement(ElementCount index) const {
BitCount64 indexBit = ElementCount64(index) * step; BitCount64 indexBit = ElementCount64(index) * step;
byte* structData = ptr + indexBit / BITS_PER_BYTE; byte* structData = ptr + indexBit / BITS_PER_BYTE;
return StructBuilder(segment, structData, return StructBuilder(segment, structData,
reinterpret_cast<WireReference*>(structData) + elementSize.data / WORDS_PER_REFERENCE, reinterpret_cast<WireReference*>(structData + structDataSize / BITS_PER_BYTE),
structDataSize, structReferenceCount, indexBit % BITS_PER_BYTE); structDataSize, structReferenceCount, indexBit % BITS_PER_BYTE);
} }
...@@ -1310,9 +1310,9 @@ Data::Builder ListBuilder::getBlobElement<Data>(ElementCount index) const { ...@@ -1310,9 +1310,9 @@ Data::Builder ListBuilder::getBlobElement<Data>(ElementCount index) const {
0 * BYTES); 0 * BYTES);
} }
ObjectBuilder ListBuilder::getObjectElement(ElementCount index, const word* defaultValue) const { ObjectBuilder ListBuilder::getObjectElement(ElementCount index) const {
return WireHelpers::getWritableObjectReference( return WireHelpers::getWritableObjectReference(
segment, reinterpret_cast<WireReference*>(ptr + index * step / BITS_PER_BYTE), defaultValue); segment, reinterpret_cast<WireReference*>(ptr + index * step / BITS_PER_BYTE), nullptr);
} }
ListReader ListBuilder::asReader() const { ListReader ListBuilder::asReader() const {
...@@ -1403,9 +1403,9 @@ Data::Reader ListReader::getBlobElement<Data>(ElementCount index) const { ...@@ -1403,9 +1403,9 @@ Data::Reader ListReader::getBlobElement<Data>(ElementCount index) const {
nullptr, 0 * BYTES); nullptr, 0 * BYTES);
} }
ObjectReader ListReader::getObjectElement(ElementCount index, const word* defaultValue) const { ObjectReader ListReader::getObjectElement(ElementCount index) const {
return WireHelpers::readObjectReference( return WireHelpers::readObjectReference(
segment, checkAlignment(ptr + index * step / BITS_PER_BYTE), defaultValue, nestingLimit); segment, checkAlignment(ptr + index * step / BITS_PER_BYTE), nullptr, nestingLimit);
} }
} // namespace internal } // namespace internal
......
...@@ -502,7 +502,7 @@ public: ...@@ -502,7 +502,7 @@ public:
ElementCount index, typename NoInfer<T>::Type value) const); ElementCount index, typename NoInfer<T>::Type value) const);
// Set the element at the given index. // Set the element at the given index.
StructBuilder getStructElement(ElementCount index, StructSize elementSize) const; StructBuilder getStructElement(ElementCount index) const;
// Get the struct element at the given index. // Get the struct element at the given index.
ListBuilder initListElement( ListBuilder initListElement(
...@@ -532,7 +532,7 @@ public: ...@@ -532,7 +532,7 @@ public:
typename T::Builder getBlobElement(ElementCount index) const; typename T::Builder getBlobElement(ElementCount index) const;
// Get the blob element. If it is not initialized, return an empty blob builder. // Get the blob element. If it is not initialized, return an empty blob builder.
ObjectBuilder getObjectElement(ElementCount index, const word* defaultValue) const; ObjectBuilder getObjectElement(ElementCount index) const;
// Gets a pointer element of arbitrary type. // Gets a pointer element of arbitrary type.
ListReader asReader() const; ListReader asReader() const;
...@@ -591,7 +591,7 @@ public: ...@@ -591,7 +591,7 @@ public:
typename T::Reader getBlobElement(ElementCount index) const; typename T::Reader getBlobElement(ElementCount index) const;
// Gets the text or data field. If it is not initialized, returns an empty blob reader. // Gets the text or data field. If it is not initialized, returns an empty blob reader.
ObjectReader getObjectElement(ElementCount index, const word* defaultValue) const; ObjectReader getObjectElement(ElementCount index) const;
// Gets a pointer element of arbitrary type. // Gets a pointer element of arbitrary type.
private: private:
......
...@@ -297,8 +297,7 @@ struct List<T, Kind::STRUCT> { ...@@ -297,8 +297,7 @@ struct List<T, Kind::STRUCT> {
inline uint size() const { return builder.size() / ELEMENTS; } inline uint size() const { return builder.size() / ELEMENTS; }
inline typename T::Builder operator[](uint index) const { inline typename T::Builder operator[](uint index) const {
return typename T::Builder(builder.getStructElement( return typename T::Builder(builder.getStructElement(index * ELEMENTS));
index * ELEMENTS, internal::structSize<T>()));
} }
typedef internal::IndexingIterator<Builder, typename T::Builder> iterator; typedef internal::IndexingIterator<Builder, typename T::Builder> iterator;
......
...@@ -49,7 +49,7 @@ internal::StructReader MessageReader::getRootInternal() { ...@@ -49,7 +49,7 @@ internal::StructReader MessageReader::getRootInternal() {
allocatedArena = true; allocatedArena = true;
} }
internal::SegmentReader* segment = arena()->tryGetSegment(SegmentId(0)); internal::SegmentReader* segment = arena()->tryGetSegment(internal::SegmentId(0));
VALIDATE_INPUT(segment != nullptr && VALIDATE_INPUT(segment != nullptr &&
segment->containsInterval(segment->getStartPtr(), segment->getStartPtr() + 1), segment->containsInterval(segment->getStartPtr(), segment->getStartPtr() + 1),
"Message did not contain a root pointer.") { "Message did not contain a root pointer.") {
...@@ -70,7 +70,7 @@ MessageBuilder::~MessageBuilder() { ...@@ -70,7 +70,7 @@ MessageBuilder::~MessageBuilder() {
internal::SegmentBuilder* MessageBuilder::getRootSegment() { internal::SegmentBuilder* MessageBuilder::getRootSegment() {
if (allocatedArena) { if (allocatedArena) {
return arena()->getSegment(SegmentId(0)); return arena()->getSegment(internal::SegmentId(0));
} else { } else {
static_assert(sizeof(internal::BuilderArena) <= sizeof(arenaSpace), static_assert(sizeof(internal::BuilderArena) <= sizeof(arenaSpace),
"arenaSpace is too small to hold a BuilderArena. Please increase it. This will break " "arenaSpace is too small to hold a BuilderArena. Please increase it. This will break "
...@@ -80,7 +80,7 @@ internal::SegmentBuilder* MessageBuilder::getRootSegment() { ...@@ -80,7 +80,7 @@ internal::SegmentBuilder* MessageBuilder::getRootSegment() {
WordCount refSize = 1 * REFERENCES * WORDS_PER_REFERENCE; WordCount refSize = 1 * REFERENCES * WORDS_PER_REFERENCE;
internal::SegmentBuilder* segment = arena()->getSegmentWithAvailable(refSize); internal::SegmentBuilder* segment = arena()->getSegmentWithAvailable(refSize);
CHECK(segment->getSegmentId() == SegmentId(0), CHECK(segment->getSegmentId() == internal::SegmentId(0),
"First allocated word of new arena was not in segment ID 0."); "First allocated word of new arena was not in segment ID 0.");
word* location = segment->allocate(refSize); word* location = segment->allocate(refSize);
CHECK(location == segment->getPtrUnchecked(0 * WORDS), CHECK(location == segment->getPtrUnchecked(0 * WORDS),
......
...@@ -37,8 +37,7 @@ namespace internal { ...@@ -37,8 +37,7 @@ namespace internal {
class BuilderArena; class BuilderArena;
} }
class Segment; class SchemaPool;
typedef Id<uint32_t, Segment> SegmentId;
// ======================================================================================= // =======================================================================================
...@@ -97,6 +96,13 @@ public: ...@@ -97,6 +96,13 @@ public:
template <typename RootType> template <typename RootType>
typename RootType::Reader getRoot(); typename RootType::Reader getRoot();
// Get the root struct of the message, interpreting it as the given struct type.
template <typename RootType>
typename RootType::Reader getRoot(const SchemaPool& pool, uint64_t typeId);
// Dynamically interpret the root struct of the message using the type with the given ID.
// RootType in this case must be DynamicStruct, and you must #include <capnproto/dynamic.h> to
// use this.
private: private:
ReaderOptions options; ReaderOptions options;
...@@ -125,8 +131,23 @@ public: ...@@ -125,8 +131,23 @@ public:
template <typename RootType> template <typename RootType>
typename RootType::Builder initRoot(); typename RootType::Builder initRoot();
// Initialize the root struct of the message as the given struct type.
template <typename RootType> template <typename RootType>
typename RootType::Builder getRoot(); typename RootType::Builder getRoot();
// Get the root struct of the message, interpreting it as the given struct type.
template <typename RootType>
typename RootType::Builder getRoot(const SchemaPool& pool, uint64_t typeId);
// Dynamically interpret the root struct of the message using the type with the given ID.
// RootType in this case must be DynamicStruct, and you must #include <capnproto/dynamic.h> to
// use this.
template <typename RootType>
typename RootType::Builder initRoot(const SchemaPool& pool, uint64_t typeId);
// Dynamically init the root struct of the message using the type with the given ID.
// RootType in this case must be DynamicStruct, and you must #include <capnproto/dynamic.h> to
// use this.
ArrayPtr<const ArrayPtr<const word>> getSegmentsForOutput(); ArrayPtr<const ArrayPtr<const word>> getSegmentsForOutput();
...@@ -270,16 +291,19 @@ inline const ReaderOptions& MessageReader::getOptions() { ...@@ -270,16 +291,19 @@ inline const ReaderOptions& MessageReader::getOptions() {
template <typename RootType> template <typename RootType>
inline typename RootType::Reader MessageReader::getRoot() { inline typename RootType::Reader MessageReader::getRoot() {
static_assert(kind<RootType>() == Kind::STRUCT, "Root type must be a Cap'n Proto struct type.");
return typename RootType::Reader(getRootInternal()); return typename RootType::Reader(getRootInternal());
} }
template <typename RootType> template <typename RootType>
inline typename RootType::Builder MessageBuilder::initRoot() { inline typename RootType::Builder MessageBuilder::initRoot() {
static_assert(kind<RootType>() == Kind::STRUCT, "Root type must be a Cap'n Proto struct type.");
return typename RootType::Builder(initRoot(internal::structSize<RootType>())); return typename RootType::Builder(initRoot(internal::structSize<RootType>()));
} }
template <typename RootType> template <typename RootType>
inline typename RootType::Builder MessageBuilder::getRoot() { inline typename RootType::Builder MessageBuilder::getRoot() {
static_assert(kind<RootType>() == Kind::STRUCT, "Root type must be a Cap'n Proto struct type.");
return typename RootType::Builder(getRoot(internal::structSize<RootType>())); return typename RootType::Builder(getRoot(internal::structSize<RootType>()));
} }
......
...@@ -219,6 +219,11 @@ struct StructNode { ...@@ -219,6 +219,11 @@ struct StructNode {
} }
struct Field { struct Field {
index @3 :UInt16;
# The index of this field within the containing struct or union's member list. This is
# redundant information, but it can be useful for the dynamic API which uses Field pointers as
# identifiers.
offset @0 :UInt32; offset @0 :UInt32;
# Offset, in units of the field's size, from the beginning of the section in which the field # Offset, in units of the field's size, from the beginning of the section in which the field
# resides. E.g. for a UInt32 field, multiply this by 4 to get the byte offset from the # resides. E.g. for a UInt32 field, multiply this by 4 to get the byte offset from the
......
...@@ -575,7 +575,7 @@ encodeSchema requestedFiles allFiles = encRoot where ...@@ -575,7 +575,7 @@ encodeSchema requestedFiles allFiles = encRoot where
, (16, encUInt16 $ structPointerCount desc) , (16, encUInt16 $ structPointerCount desc)
, (32, encUInt16 (fieldSizeEnum preferredListEncoding::Word16)) , (32, encUInt16 (fieldSizeEnum preferredListEncoding::Word16))
] ]
ptrValues = [ (0, encStructList memberSize $ map encMember $ ptrValues = [ (0, encStructList memberSize $ zipWith encMember [0::Word16 ..] $
sortMembers $ structMembers desc) ] sortMembers $ structMembers desc) ]
preferredListEncoding = case (structDataSize desc, structPointerCount desc) of preferredListEncoding = case (structDataSize desc, structPointerCount desc) of
...@@ -597,7 +597,7 @@ encodeSchema requestedFiles allFiles = encRoot where ...@@ -597,7 +597,7 @@ encodeSchema requestedFiles allFiles = encRoot where
selectFieldOrUnion _ = Nothing selectFieldOrUnion _ = Nothing
memberSize = (DataSectionWords 1, 3) memberSize = (DataSectionWords 1, 3)
encMember (codeOrder, (_, DescField field)) = (dataValues2, ptrValues2) where encMember index (codeOrder, (_, DescField field)) = (dataValues2, ptrValues2) where
dataValues2 = [ (0, encUInt16 $ fieldNumber field) dataValues2 = [ (0, encUInt16 $ fieldNumber field)
, (16, encUInt16 codeOrder) , (16, encUInt16 codeOrder)
, (32, encUInt16 (0::Word16)) -- discriminant , (32, encUInt16 (0::Word16)) -- discriminant
...@@ -608,7 +608,8 @@ encodeSchema requestedFiles allFiles = encRoot where ...@@ -608,7 +608,8 @@ encodeSchema requestedFiles allFiles = encRoot where
] ]
-- StructNode.Field -- StructNode.Field
dataValues3 = [ (0, encUInt32 $ offsetToInt $ fieldOffset field) ] dataValues3 = [ (0, encUInt32 $ offsetToInt $ fieldOffset field)
, (32, encUInt16 index) ]
ptrValues3 = [ (0, encStruct typeSize $ encType $ fieldType field) ptrValues3 = [ (0, encStruct typeSize $ encType $ fieldType field)
, (1, encStruct valueSize $ encValue (fieldType field) $ , (1, encStruct valueSize $ encValue (fieldType field) $
fieldDefaultValue field) fieldDefaultValue field)
...@@ -620,7 +621,7 @@ encodeSchema requestedFiles allFiles = encRoot where ...@@ -620,7 +621,7 @@ encodeSchema requestedFiles allFiles = encRoot where
offsetToInt (InlineCompositeOffset {}) = offsetToInt (InlineCompositeOffset {}) =
error "Inline types not currently supported by codegen plugins." error "Inline types not currently supported by codegen plugins."
encMember (codeOrder, (_, DescUnion union)) = (dataValues2, ptrValues2) where encMember _ (codeOrder, (_, DescUnion union)) = (dataValues2, ptrValues2) where
dataValues2 = [ (0, encUInt16 $ unionNumber union) dataValues2 = [ (0, encUInt16 $ unionNumber union)
, (16, encUInt16 codeOrder) , (16, encUInt16 codeOrder)
, (32, encUInt16 (1::Word16)) -- discriminant , (32, encUInt16 (1::Word16)) -- discriminant
...@@ -632,9 +633,9 @@ encodeSchema requestedFiles allFiles = encRoot where ...@@ -632,9 +633,9 @@ encodeSchema requestedFiles allFiles = encRoot where
-- StructNode.Union -- StructNode.Union
dataValues3 = [ (0, encUInt32 $ unionTagOffset union) ] dataValues3 = [ (0, encUInt32 $ unionTagOffset union) ]
ptrValues3 = [ (0, encStructList memberSize $ map encMember $ sortMembers $ ptrValues3 = [ (0, encStructList memberSize $ zipWith encMember [0..] $ sortMembers $
unionMembers union) ] unionMembers union) ]
encMember _ = error "Not a field or union?" encMember _ _ = error "Not a field or union?"
enumNodeSize = (DataSectionWords 0, 1) enumNodeSize = (DataSectionWords 0, 1)
encEnumNode desc = (dataValues, ptrValues) where encEnumNode desc = (dataValues, ptrValues) where
......
...@@ -157,6 +157,8 @@ public: ...@@ -157,6 +157,8 @@ public:
{{/fieldIsPrimitive}} {{/fieldIsPrimitive}}
{{#fieldIsGenericObject}} {{#fieldIsGenericObject}}
template <typename T> inline typename T::Reader get{{fieldTitleCase}}(); template <typename T> inline typename T::Reader get{{fieldTitleCase}}();
template <typename T, typename... Params> inline typename T::Reader get{{fieldTitleCase}}(
const ::capnproto::SchemaPool& pool, Params&&... params);
{{/fieldIsGenericObject}} {{/fieldIsGenericObject}}
{{/typeFields}} {{/typeFields}}
private: private:
...@@ -209,6 +211,11 @@ public: ...@@ -209,6 +211,11 @@ public:
template <typename T> inline void set{{fieldTitleCase}}(typename T::Reader value); template <typename T> inline void set{{fieldTitleCase}}(typename T::Reader value);
template <typename T> inline typename T::Builder init{{fieldTitleCase}}(); template <typename T> inline typename T::Builder init{{fieldTitleCase}}();
template <typename T> inline typename T::Builder init{{fieldTitleCase}}(unsigned int size); template <typename T> inline typename T::Builder init{{fieldTitleCase}}(unsigned int size);
template <typename T, typename... Params> inline typename T::Builder get{{fieldTitleCase}}(
const ::capnproto::SchemaPool& pool, Params&&... params);
template <typename T, typename... Params> inline typename T::Builder init{{fieldTitleCase}}(
const ::capnproto::SchemaPool& pool, Params&&... params);
{{/fieldIsGenericObject}} {{/fieldIsGenericObject}}
{{/typeFields}} {{/typeFields}}
private: private:
...@@ -394,6 +401,42 @@ inline typename T::Builder {{typeFullName}}::Builder::init{{fieldTitleCase}}(uns ...@@ -394,6 +401,42 @@ inline typename T::Builder {{typeFullName}}::Builder::init{{fieldTitleCase}}(uns
_builder, {{fieldOffset}} * ::capnproto::REFERENCES, size); _builder, {{fieldOffset}} * ::capnproto::REFERENCES, size);
} }
template <typename T, typename... Params>
inline typename T::Reader {{typeFullName}}::Reader::get{{fieldTitleCase}}(
const ::capnproto::SchemaPool& pool, Params&&... params) {
{{#fieldUnion}}
CAPNPROTO_INLINE_DPRECOND(which() == {{unionTitleCase}}::{{fieldUpperCase}},
"Must check which() before get()ing a union member.");
{{/fieldUnion}}
return ::capnproto::internal::PointerHelpers<T>::get(
_reader, {{fieldOffset}} * ::capnproto::REFERENCES,
pool, ::capnproto::forward<Params>(params)...);
}
template <typename T, typename... Params>
inline typename T::Builder {{typeFullName}}::Builder::get{{fieldTitleCase}}(
const ::capnproto::SchemaPool& pool, Params&&... params) {
{{#fieldUnion}}
CAPNPROTO_INLINE_DPRECOND(which() == {{unionTitleCase}}::{{fieldUpperCase}},
"Must check which() before get()ing a union member.");
{{/fieldUnion}}
return ::capnproto::internal::PointerHelpers<T>::get(
_builder, {{fieldOffset}} * ::capnproto::REFERENCES,
pool, ::capnproto::forward<Params>(params)...);
}
template <typename T, typename... Params>
inline typename T::Builder {{typeFullName}}::Builder::init{{fieldTitleCase}}(
const ::capnproto::SchemaPool& pool, Params&&... params) {
{{#fieldUnion}}
_builder.setDataField<{{unionTitleCase}}::Which>(
{{unionTagOffset}} * ::capnproto::ELEMENTS, {{unionTitleCase}}::{{fieldUpperCase}});
{{/fieldUnion}}
return ::capnproto::internal::PointerHelpers<T>::init(
_builder, {{fieldOffset}} * ::capnproto::REFERENCES,
pool, ::capnproto::forward<Params>(params)...);
}
{{/fieldIsGenericObject}} {{/fieldIsGenericObject}}
{{/typeFields}} {{/typeFields}}
{{/typeStructOrUnion}} {{/typeStructOrUnion}}
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment