Convert BeginReadData...() to use the new user pointer handling (see r285350).

R=darin@chromium.org

Review URL: https://codereview.chromium.org/416203002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@285601 0039d316-1c4b-4281-b951-d872f2087c98
parent 85a071fb
......@@ -412,8 +412,7 @@ MojoResult Core::BeginReadData(MojoHandle data_pipe_consumer_handle,
if (!dispatcher)
return MOJO_RESULT_INVALID_ARGUMENT;
return dispatcher->BeginReadData(buffer.GetPointerUnsafe(),
buffer_num_bytes.GetPointerUnsafe(), flags);
return dispatcher->BeginReadData(buffer, buffer_num_bytes, flags);
}
MojoResult Core::EndReadData(MojoHandle data_pipe_consumer_handle,
......
......@@ -122,8 +122,8 @@ class MockDispatcher : public Dispatcher {
}
virtual MojoResult BeginReadDataImplNoLock(
const void** /*buffer*/,
uint32_t* /*buffer_num_bytes*/,
UserPointer<const void*> /*buffer*/,
UserPointer<uint32_t> /*buffer_num_bytes*/,
MojoReadDataFlags /*flags*/) OVERRIDE {
info_->IncrementBeginReadDataCallCount();
lock().AssertAcquired();
......
......@@ -286,20 +286,25 @@ MojoResult DataPipe::ConsumerQueryData(uint32_t* num_bytes) {
return ConsumerQueryDataImplNoLock(num_bytes);
}
MojoResult DataPipe::ConsumerBeginReadData(const void** buffer,
uint32_t* buffer_num_bytes,
bool all_or_none) {
MojoResult DataPipe::ConsumerBeginReadData(
UserPointer<const void*> buffer,
UserPointer<uint32_t> buffer_num_bytes,
bool all_or_none) {
base::AutoLock locker(lock_);
DCHECK(has_local_consumer_no_lock());
if (consumer_in_two_phase_read_no_lock())
return MOJO_RESULT_BUSY;
if (all_or_none && *buffer_num_bytes % element_num_bytes_ != 0)
return MOJO_RESULT_INVALID_ARGUMENT;
uint32_t min_num_bytes_to_read = 0;
if (all_or_none) {
min_num_bytes_to_read = buffer_num_bytes.Get();
if (min_num_bytes_to_read % element_num_bytes_ != 0)
return MOJO_RESULT_INVALID_ARGUMENT;
}
MojoResult rv = ConsumerBeginReadDataImplNoLock(buffer, buffer_num_bytes,
all_or_none);
min_num_bytes_to_read);
if (rv != MOJO_RESULT_OK)
return rv;
DCHECK(consumer_in_two_phase_read_no_lock());
......
......@@ -77,9 +77,8 @@ class MOJO_SYSTEM_IMPL_EXPORT DataPipe :
MojoResult ConsumerDiscardData(uint32_t* num_bytes,
bool all_or_none);
MojoResult ConsumerQueryData(uint32_t* num_bytes);
// This does not validate its arguments.
MojoResult ConsumerBeginReadData(const void** buffer,
uint32_t* buffer_num_bytes,
MojoResult ConsumerBeginReadData(UserPointer<const void*> buffer,
UserPointer<uint32_t> buffer_num_bytes,
bool all_or_none);
MojoResult ConsumerEndReadData(uint32_t num_bytes_read);
MojoResult ConsumerAddWaiter(Waiter* waiter,
......@@ -119,9 +118,10 @@ class MOJO_SYSTEM_IMPL_EXPORT DataPipe :
bool all_or_none) = 0;
// |*num_bytes| will be a nonzero multiple of |element_num_bytes_|.
virtual MojoResult ConsumerQueryDataImplNoLock(uint32_t* num_bytes) = 0;
virtual MojoResult ConsumerBeginReadDataImplNoLock(const void** buffer,
uint32_t* buffer_num_bytes,
bool all_or_none) = 0;
virtual MojoResult ConsumerBeginReadDataImplNoLock(
UserPointer<const void*> buffer,
UserPointer<uint32_t> buffer_num_bytes,
uint32_t min_num_bytes_to_read) = 0;
virtual MojoResult ConsumerEndReadDataImplNoLock(uint32_t num_bytes_read) = 0;
// Note: A consumer should not be writable during a two-phase read.
virtual HandleSignalsState ConsumerGetHandleSignalsStateNoLock() const = 0;
......
......@@ -83,15 +83,11 @@ MojoResult DataPipeConsumerDispatcher::ReadDataImplNoLock(
}
MojoResult DataPipeConsumerDispatcher::BeginReadDataImplNoLock(
const void** buffer,
uint32_t* buffer_num_bytes,
UserPointer<const void*> buffer,
UserPointer<uint32_t> buffer_num_bytes,
MojoReadDataFlags flags) {
lock().AssertAcquired();
if (!VerifyUserPointerWithCount<const void*>(buffer, 1))
return MOJO_RESULT_INVALID_ARGUMENT;
if (!VerifyUserPointer<uint32_t>(buffer_num_bytes))
return MOJO_RESULT_INVALID_ARGUMENT;
// These flags may not be used in two-phase mode.
if ((flags & MOJO_READ_DATA_FLAG_DISCARD) ||
(flags & MOJO_READ_DATA_FLAG_QUERY))
......
......@@ -40,9 +40,10 @@ class MOJO_SYSTEM_IMPL_EXPORT DataPipeConsumerDispatcher : public Dispatcher {
virtual MojoResult ReadDataImplNoLock(void* elements,
uint32_t* num_bytes,
MojoReadDataFlags flags) OVERRIDE;
virtual MojoResult BeginReadDataImplNoLock(const void** buffer,
uint32_t* buffer_num_bytes,
MojoReadDataFlags flags) OVERRIDE;
virtual MojoResult BeginReadDataImplNoLock(
UserPointer<const void*> buffer,
UserPointer<uint32_t> buffer_num_bytes,
MojoReadDataFlags flags) OVERRIDE;
virtual MojoResult EndReadDataImplNoLock(uint32_t num_bytes_read) OVERRIDE;
virtual MojoResult AddWaiterImplNoLock(Waiter* waiter,
MojoHandleSignals signals,
......
......@@ -174,8 +174,8 @@ MojoResult Dispatcher::ReadData(void* elements,
return ReadDataImplNoLock(elements, num_bytes, flags);
}
MojoResult Dispatcher::BeginReadData(const void** buffer,
uint32_t* buffer_num_bytes,
MojoResult Dispatcher::BeginReadData(UserPointer<const void*> buffer,
UserPointer<uint32_t> buffer_num_bytes,
MojoReadDataFlags flags) {
base::AutoLock locker(lock_);
if (is_closed_)
......@@ -311,9 +311,10 @@ MojoResult Dispatcher::ReadDataImplNoLock(void* /*elements*/,
return MOJO_RESULT_INVALID_ARGUMENT;
}
MojoResult Dispatcher::BeginReadDataImplNoLock(const void** /*buffer*/,
uint32_t* /*buffer_num_bytes*/,
MojoReadDataFlags /*flags*/) {
MojoResult Dispatcher::BeginReadDataImplNoLock(
UserPointer<const void*> /*buffer*/,
UserPointer<uint32_t> /*buffer_num_bytes*/,
MojoReadDataFlags /*flags*/) {
lock_.AssertAcquired();
DCHECK(!is_closed_);
// By default, not supported. Only needed for data pipe dispatchers.
......
......@@ -102,8 +102,8 @@ class MOJO_SYSTEM_IMPL_EXPORT Dispatcher :
MojoResult ReadData(void* elements,
uint32_t* num_bytes,
MojoReadDataFlags flags);
MojoResult BeginReadData(const void** buffer,
uint32_t* buffer_num_bytes,
MojoResult BeginReadData(UserPointer<const void*> buffer,
UserPointer<uint32_t> buffer_num_bytes,
MojoReadDataFlags flags);
MojoResult EndReadData(uint32_t num_bytes_read);
// |options| may be null. |new_dispatcher| must not be null, but
......@@ -233,9 +233,10 @@ class MOJO_SYSTEM_IMPL_EXPORT Dispatcher :
virtual MojoResult ReadDataImplNoLock(void* elements,
uint32_t* num_bytes,
MojoReadDataFlags flags);
virtual MojoResult BeginReadDataImplNoLock(const void** buffer,
uint32_t* buffer_num_bytes,
MojoReadDataFlags flags);
virtual MojoResult BeginReadDataImplNoLock(
UserPointer<const void*> buffer,
UserPointer<uint32_t> buffer_num_bytes,
MojoReadDataFlags flags);
virtual MojoResult EndReadDataImplNoLock(uint32_t num_bytes_read);
virtual MojoResult DuplicateBufferHandleImplNoLock(
const MojoDuplicateBufferHandleOptions* options,
......
......@@ -60,7 +60,8 @@ TEST(DispatcherTest, Basic) {
EXPECT_EQ(MOJO_RESULT_INVALID_ARGUMENT,
d->ReadData(NULL, NULL, MOJO_READ_DATA_FLAG_NONE));
EXPECT_EQ(MOJO_RESULT_INVALID_ARGUMENT,
d->BeginReadData(NULL, NULL, MOJO_READ_DATA_FLAG_NONE));
d->BeginReadData(NullUserPointer(), NullUserPointer(),
MOJO_READ_DATA_FLAG_NONE));
EXPECT_EQ(MOJO_RESULT_INVALID_ARGUMENT,
d->EndReadData(0));
Waiter w;
......@@ -88,7 +89,8 @@ TEST(DispatcherTest, Basic) {
EXPECT_EQ(MOJO_RESULT_INVALID_ARGUMENT,
d->ReadData(NULL, NULL, MOJO_READ_DATA_FLAG_NONE));
EXPECT_EQ(MOJO_RESULT_INVALID_ARGUMENT,
d->BeginReadData(NULL, NULL, MOJO_READ_DATA_FLAG_NONE));
d->BeginReadData(NullUserPointer(), NullUserPointer(),
MOJO_READ_DATA_FLAG_NONE));
EXPECT_EQ(MOJO_RESULT_INVALID_ARGUMENT,
d->EndReadData(0));
EXPECT_EQ(MOJO_RESULT_INVALID_ARGUMENT,
......@@ -174,7 +176,8 @@ class ThreadSafetyStressThread : public base::SimpleThread {
break;
case BEGIN_READ_DATA:
EXPECT_EQ(MOJO_RESULT_INVALID_ARGUMENT,
dispatcher_->BeginReadData(NULL, NULL,
dispatcher_->BeginReadData(NullUserPointer(),
NullUserPointer(),
MOJO_READ_DATA_FLAG_NONE));
break;
case END_READ_DATA:
......
......@@ -240,11 +240,11 @@ MojoResult LocalDataPipe::ConsumerQueryDataImplNoLock(uint32_t* num_bytes) {
}
MojoResult LocalDataPipe::ConsumerBeginReadDataImplNoLock(
const void** buffer,
uint32_t* buffer_num_bytes,
bool all_or_none) {
UserPointer<const void*> buffer,
UserPointer<uint32_t> buffer_num_bytes,
uint32_t min_num_bytes_to_read) {
size_t max_num_bytes_to_read = GetMaxNumBytesToReadNoLock();
if (all_or_none && *buffer_num_bytes > max_num_bytes_to_read) {
if (min_num_bytes_to_read > max_num_bytes_to_read) {
// Don't return "should wait" since you can't wait for a specified amount of
// data.
return producer_open_no_lock() ? MOJO_RESULT_OUT_OF_RANGE :
......@@ -257,8 +257,8 @@ MojoResult LocalDataPipe::ConsumerBeginReadDataImplNoLock(
MOJO_RESULT_FAILED_PRECONDITION;
}
*buffer = buffer_.get() + start_index_;
*buffer_num_bytes = static_cast<uint32_t>(max_num_bytes_to_read);
buffer.Put(buffer_.get() + start_index_);
buffer_num_bytes.Put(static_cast<uint32_t>(max_num_bytes_to_read));
set_consumer_two_phase_max_num_bytes_read_no_lock(
static_cast<uint32_t>(max_num_bytes_to_read));
return MOJO_RESULT_OK;
......
......@@ -49,9 +49,10 @@ class MOJO_SYSTEM_IMPL_EXPORT LocalDataPipe : public DataPipe {
virtual MojoResult ConsumerDiscardDataImplNoLock(uint32_t* num_bytes,
bool all_or_none) OVERRIDE;
virtual MojoResult ConsumerQueryDataImplNoLock(uint32_t* num_bytes) OVERRIDE;
virtual MojoResult ConsumerBeginReadDataImplNoLock(const void** buffer,
uint32_t* buffer_num_bytes,
bool all_or_none) OVERRIDE;
virtual MojoResult ConsumerBeginReadDataImplNoLock(
UserPointer<const void*> buffer,
UserPointer<uint32_t> buffer_num_bytes,
uint32_t min_num_bytes_to_read) OVERRIDE;
virtual MojoResult ConsumerEndReadDataImplNoLock(
uint32_t num_bytes_read) OVERRIDE;
virtual HandleSignalsState
......
......@@ -270,7 +270,8 @@ TEST(LocalDataPipeTest, BasicProducerWaiting) {
const void* read_buffer = NULL;
num_bytes = 0u;
EXPECT_EQ(MOJO_RESULT_OK,
dp->ConsumerBeginReadData(&read_buffer, &num_bytes, false));
dp->ConsumerBeginReadData(MakeUserPointer(&read_buffer),
MakeUserPointer(&num_bytes), false));
EXPECT_TRUE(read_buffer != NULL);
// Since we only read one element (after having written three in all), the
// two-phase read should only allow us to read one. This checks an
......@@ -442,7 +443,8 @@ TEST(LocalDataPipeTest, BasicConsumerWaiting) {
const void* read_buffer = NULL;
num_bytes = static_cast<uint32_t>(2u * sizeof(elements[0]));
EXPECT_EQ(MOJO_RESULT_OK,
dp->ConsumerBeginReadData(&read_buffer, &num_bytes, true));
dp->ConsumerBeginReadData(MakeUserPointer(&read_buffer),
MakeUserPointer(&num_bytes), true));
EXPECT_TRUE(read_buffer != NULL);
EXPECT_EQ(static_cast<uint32_t>(2u * sizeof(elements[0])), num_bytes);
const int32_t* read_elements = static_cast<const int32_t*>(read_buffer);
......@@ -461,7 +463,8 @@ TEST(LocalDataPipeTest, BasicConsumerWaiting) {
read_buffer = NULL;
num_bytes = static_cast<uint32_t>(3u * sizeof(elements[0]));
EXPECT_EQ(MOJO_RESULT_OK,
dp->ConsumerBeginReadData(&read_buffer, &num_bytes, false));
dp->ConsumerBeginReadData(MakeUserPointer(&read_buffer),
MakeUserPointer(&num_bytes), false));
EXPECT_TRUE(read_buffer != NULL);
EXPECT_EQ(static_cast<uint32_t>(1u * sizeof(elements[0])), num_bytes);
read_elements = static_cast<const int32_t*>(read_buffer);
......@@ -566,7 +569,8 @@ TEST(LocalDataPipeTest, BasicTwoPhaseWaiting) {
num_bytes = static_cast<uint32_t>(1u * sizeof(int32_t));
const void* read_ptr = NULL;
EXPECT_EQ(MOJO_RESULT_OK,
dp->ConsumerBeginReadData(&read_ptr, &num_bytes, false));
dp->ConsumerBeginReadData(MakeUserPointer(&read_ptr),
MakeUserPointer(&num_bytes), false));
EXPECT_TRUE(read_ptr != NULL);
EXPECT_EQ(static_cast<uint32_t>(1u * sizeof(int32_t)), num_bytes);
......@@ -1122,7 +1126,8 @@ TEST(LocalDataPipeTest, TwoPhaseAllOrNone) {
num_bytes = 20u * sizeof(int32_t);
const void* read_ptr = NULL;
EXPECT_EQ(MOJO_RESULT_OUT_OF_RANGE,
dp->ConsumerBeginReadData(&read_ptr, &num_bytes, true));
dp->ConsumerBeginReadData(MakeUserPointer(&read_ptr),
MakeUserPointer(&num_bytes), true));
// Write half (two-phase).
num_bytes = 5u * sizeof(int32_t);
......@@ -1141,13 +1146,15 @@ TEST(LocalDataPipeTest, TwoPhaseAllOrNone) {
num_bytes = 1u;
read_ptr = NULL;
EXPECT_EQ(MOJO_RESULT_INVALID_ARGUMENT,
dp->ConsumerBeginReadData(&read_ptr, &num_bytes, true));
dp->ConsumerBeginReadData(MakeUserPointer(&read_ptr),
MakeUserPointer(&num_bytes), true));
// Read one (two-phase).
num_bytes = 1u * sizeof(int32_t);
read_ptr = NULL;
EXPECT_EQ(MOJO_RESULT_OK,
dp->ConsumerBeginReadData(&read_ptr, &num_bytes, true));
dp->ConsumerBeginReadData(MakeUserPointer(&read_ptr),
MakeUserPointer(&num_bytes), true));
EXPECT_GE(num_bytes, 1u * sizeof(int32_t));
EXPECT_EQ(0, static_cast<const int32_t*>(read_ptr)[0]);
EXPECT_EQ(MOJO_RESULT_OK, dp->ConsumerEndReadData(1u * sizeof(int32_t)));
......@@ -1181,7 +1188,8 @@ TEST(LocalDataPipeTest, TwoPhaseAllOrNone) {
num_bytes = 10u * sizeof(int32_t);
read_ptr = NULL;
EXPECT_EQ(MOJO_RESULT_OUT_OF_RANGE,
dp->ConsumerBeginReadData(&read_ptr, &num_bytes, true));
dp->ConsumerBeginReadData(MakeUserPointer(&read_ptr),
MakeUserPointer(&num_bytes), true));
// Close the producer.
dp->ProducerClose();
......@@ -1190,7 +1198,8 @@ TEST(LocalDataPipeTest, TwoPhaseAllOrNone) {
num_bytes = 9u * sizeof(int32_t);
read_ptr = NULL;
EXPECT_EQ(MOJO_RESULT_OK,
dp->ConsumerBeginReadData(&read_ptr, &num_bytes, true));
dp->ConsumerBeginReadData(MakeUserPointer(&read_ptr),
MakeUserPointer(&num_bytes), true));
EXPECT_GE(num_bytes, 9u * sizeof(int32_t));
EXPECT_EQ(1, static_cast<const int32_t*>(read_ptr)[0]);
EXPECT_EQ(2, static_cast<const int32_t*>(read_ptr)[1]);
......@@ -1207,7 +1216,8 @@ TEST(LocalDataPipeTest, TwoPhaseAllOrNone) {
num_bytes = 2u * sizeof(int32_t);
read_ptr = NULL;
EXPECT_EQ(MOJO_RESULT_FAILED_PRECONDITION,
dp->ConsumerBeginReadData(&read_ptr, &num_bytes, true));
dp->ConsumerBeginReadData(MakeUserPointer(&read_ptr),
MakeUserPointer(&num_bytes), true));
dp->ConsumerClose();
}
......@@ -1275,7 +1285,8 @@ TEST(LocalDataPipeTest, WrapAround) {
const void* read_buffer_ptr = NULL;
num_bytes = 0u;
EXPECT_EQ(MOJO_RESULT_OK,
dp->ConsumerBeginReadData(&read_buffer_ptr, &num_bytes, false));
dp->ConsumerBeginReadData(MakeUserPointer(&read_buffer_ptr),
MakeUserPointer(&num_bytes), false));
EXPECT_TRUE(read_buffer_ptr != NULL);
EXPECT_EQ(90u, num_bytes);
EXPECT_EQ(MOJO_RESULT_OK, dp->ConsumerEndReadData(0u));
......@@ -1339,7 +1350,8 @@ TEST(LocalDataPipeTest, CloseWriteRead) {
const void* read_buffer_ptr = NULL;
num_bytes = 0u;
EXPECT_EQ(MOJO_RESULT_OK,
dp->ConsumerBeginReadData(&read_buffer_ptr, &num_bytes, false));
dp->ConsumerBeginReadData(MakeUserPointer(&read_buffer_ptr),
MakeUserPointer(&num_bytes), false));
EXPECT_TRUE(read_buffer_ptr != NULL);
EXPECT_EQ(2u * kTestDataSize, num_bytes);
......@@ -1354,7 +1366,8 @@ TEST(LocalDataPipeTest, CloseWriteRead) {
read_buffer_ptr = NULL;
num_bytes = 0u;
EXPECT_EQ(MOJO_RESULT_OK,
dp->ConsumerBeginReadData(&read_buffer_ptr, &num_bytes, false));
dp->ConsumerBeginReadData(MakeUserPointer(&read_buffer_ptr),
MakeUserPointer(&num_bytes), false));
EXPECT_TRUE(read_buffer_ptr != NULL);
EXPECT_EQ(kTestDataSize, num_bytes);
......@@ -1385,7 +1398,8 @@ TEST(LocalDataPipeTest, CloseWriteRead) {
const void* read_buffer_ptr = NULL;
num_bytes = 0u;
EXPECT_EQ(MOJO_RESULT_OK,
dp->ConsumerBeginReadData(&read_buffer_ptr, &num_bytes, false));
dp->ConsumerBeginReadData(MakeUserPointer(&read_buffer_ptr),
MakeUserPointer(&num_bytes), false));
EXPECT_TRUE(read_buffer_ptr != NULL);
EXPECT_EQ(kTestDataSize, num_bytes);
......@@ -1462,7 +1476,8 @@ TEST(LocalDataPipeTest, CloseWriteRead) {
const void* read_buffer_ptr = NULL;
num_bytes = 0u;
EXPECT_EQ(MOJO_RESULT_FAILED_PRECONDITION,
dp->ConsumerBeginReadData(&read_buffer_ptr, &num_bytes, false));
dp->ConsumerBeginReadData(MakeUserPointer(&read_buffer_ptr),
MakeUserPointer(&num_bytes), false));
// Ditto for discard.
num_bytes = 10u;
......@@ -1559,7 +1574,8 @@ TEST(LocalDataPipeTest, TwoPhaseMoreInvalidArguments) {
num_bytes = 0u;
const void* read_ptr = NULL;
EXPECT_EQ(MOJO_RESULT_OK,
dp->ConsumerBeginReadData(&read_ptr, &num_bytes, false));
dp->ConsumerBeginReadData(MakeUserPointer(&read_ptr),
MakeUserPointer(&num_bytes), false));
EXPECT_EQ(MOJO_RESULT_INVALID_ARGUMENT,
dp->ConsumerEndReadData(
num_bytes + static_cast<uint32_t>(sizeof(int32_t))));
......@@ -1574,7 +1590,8 @@ TEST(LocalDataPipeTest, TwoPhaseMoreInvalidArguments) {
num_bytes = 0u;
read_ptr = NULL;
EXPECT_EQ(MOJO_RESULT_OK,
dp->ConsumerBeginReadData(&read_ptr, &num_bytes, false));
dp->ConsumerBeginReadData(MakeUserPointer(&read_ptr),
MakeUserPointer(&num_bytes), false));
EXPECT_EQ(1u * sizeof(int32_t), num_bytes);
EXPECT_EQ(123, static_cast<const int32_t*>(read_ptr)[0]);
EXPECT_EQ(MOJO_RESULT_INVALID_ARGUMENT, dp->ConsumerEndReadData(1u));
......@@ -1618,7 +1635,8 @@ TEST(LocalDataPipeTest, DISABLED_MayDiscardTwoPhaseConsistent) {
const void* read_ptr = NULL;
num_bytes = 2u;
EXPECT_EQ(MOJO_RESULT_OK,
dp->ConsumerBeginReadData(&read_ptr, &num_bytes, false));
dp->ConsumerBeginReadData(MakeUserPointer(&read_ptr),
MakeUserPointer(&num_bytes), false));
EXPECT_EQ(2u, num_bytes);
EXPECT_EQ('a', static_cast<const char*>(read_ptr)[0]);
EXPECT_EQ('b', static_cast<const char*>(read_ptr)[1]);
......@@ -1648,7 +1666,8 @@ TEST(LocalDataPipeTest, DISABLED_MayDiscardTwoPhaseConsistent) {
read_ptr = NULL;
num_bytes = 2u;
EXPECT_EQ(MOJO_RESULT_OK,
dp->ConsumerBeginReadData(&read_ptr, &num_bytes, false));
dp->ConsumerBeginReadData(MakeUserPointer(&read_ptr),
MakeUserPointer(&num_bytes), false));
EXPECT_EQ(2u, num_bytes);
EXPECT_EQ('x', static_cast<const char*>(read_ptr)[0]);
EXPECT_EQ('y', static_cast<const char*>(read_ptr)[1]);
......
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