Skip to content

Commit ae06ac7

Browse files
gf712gf712
committed
Address review comments
1 parent 9cd8b59 commit ae06ac7

2 files changed

Lines changed: 61 additions & 41 deletions

File tree

‎src/runtime/modules/BuiltinsModule.cpp‎

Lines changed: 17 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -69,8 +69,8 @@ namespace {
6969

7070
PyResult<PyObject *> print(const PyTuple *args, const PyDict *kwargs, Interpreter &interpreter)
7171
{
72-
std::string separator = " ";
73-
std::string end = "\n";
72+
auto *separator = PyString::create(" ").unwrap();
73+
auto *end = PyString::create("\n").unwrap();
7474
// TODO: handle error case?
7575
PyObject *file =
7676
PyObject::from(interpreter.get_imported_module(PyString::create("sys").unwrap())
@@ -94,7 +94,7 @@ PyResult<PyObject *> print(const PyTuple *args, const PyDict *kwargs, Interprete
9494
return Err(type_error(
9595
"sep must be None or a string, not {}", obj.unwrap()->type()->name()));
9696
}
97-
separator = std::get<String>(maybe_str).s;
97+
separator = PyString::create(std::get<String>(maybe_str).s).unwrap();
9898
}
9999
if (auto it = kwargs->map().find(end_keyword); it != kwargs->map().end()) {
100100
auto maybe_str = it->second;
@@ -105,7 +105,7 @@ PyResult<PyObject *> print(const PyTuple *args, const PyDict *kwargs, Interprete
105105
return Err(type_error(
106106
"end must be None or a string, not {}", obj.unwrap()->type()->name()));
107107
}
108-
end = std::get<String>(maybe_str).s;
108+
end = PyString::create(std::get<String>(maybe_str).s).unwrap();
109109
}
110110
}
111111
auto file_write_ = file->get_method(PyString::create("write").unwrap());
@@ -119,6 +119,11 @@ PyResult<PyObject *> print(const PyTuple *args, const PyDict *kwargs, Interprete
119119
auto arg_it = args->begin();
120120
auto arg_it_end = args->end();
121121
if (arg_it == arg_it_end) {
122+
if (auto result =
123+
file_write->call(PyTuple::create(PyString::create(end).unwrap()).unwrap(), nullptr);
124+
result.is_err()) {
125+
return result;
126+
}
122127
if (flush) {
123128
auto file_flush_ = file->get_method(PyString::create("flush").unwrap());
124129
if (file_flush_.is_err()) { return file_flush_; }
@@ -138,6 +143,10 @@ PyResult<PyObject *> print(const PyTuple *args, const PyDict *kwargs, Interprete
138143
result.is_err()) {
139144
return result;
140145
}
146+
if (auto result = file_write->call(PyTuple::create(separator).unwrap(), nullptr);
147+
result.is_err()) {
148+
return result;
149+
}
141150
std::advance(arg_it, 1);
142151
}
143152

@@ -151,12 +160,10 @@ PyResult<PyObject *> print(const PyTuple *args, const PyDict *kwargs, Interprete
151160
return result;
152161
}
153162

154-
if (!end.empty()) {
155-
if (auto result =
156-
file_write->call(PyTuple::create(PyString::create(end).unwrap()).unwrap(), nullptr);
157-
result.is_err()) {
158-
return result;
159-
}
163+
if (auto result =
164+
file_write->call(PyTuple::create(PyString::create(end).unwrap()).unwrap(), nullptr);
165+
result.is_err()) {
166+
return result;
160167
}
161168
if (flush) {
162169
auto file_flush_ = file->get_method(PyString::create("flush").unwrap());

‎src/runtime/modules/IOModule.cpp‎

Lines changed: 44 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -1343,25 +1343,17 @@ class BufferedWriter
13431343
});
13441344
}
13451345

1346-
PyResult<PyObject *> raw_write()
1347-
{
1348-
std::array<char, BUFSIZ> to_write;
1349-
1350-
while (true) {
1351-
// very innefficient - we shouldn't be getting data from buffer, create an intermediate
1352-
// bytes buffer copy just to then get a view of it.
1353-
auto written = buffer->sgetn(to_write.data(), BUFSIZ);
1354-
if (written <= 0) { break; }
1355-
auto *raw_bytes = bit_cast<std::byte *>(to_write.data());
1356-
auto bytes = PyBytes::create(Bytes{ .b = { raw_bytes, raw_bytes + written } });
1357-
auto memobj = PyMemoryView::create(bytes.unwrap());
1358-
if (memobj.is_err()) { return memobj; }
1359-
return raw->get_method(PyString::create("write").unwrap())
1360-
.and_then([memobj](PyObject *write) {
1361-
return write->call(PyTuple::create(memobj.unwrap()).unwrap(), nullptr);
1362-
});
1363-
}
1364-
return Ok(py_none());
1346+
PyResult<PyObject *> raw_write(const char *data, size_t len)
1347+
{
1348+
auto *raw_bytes = bit_cast<std::byte *>(data);
1349+
auto bytes = PyBytes::create(Bytes{ .b = { raw_bytes, raw_bytes + len } });
1350+
auto memobj = PyMemoryView::create(bytes.unwrap());
1351+
if (memobj.is_err()) { return memobj; }
1352+
return raw->get_method(PyString::create("write").unwrap())
1353+
.and_then([memobj](PyObject *write) {
1354+
return write->call(PyTuple::create(memobj.unwrap()).unwrap(), nullptr);
1355+
})
1356+
.and_then([](auto) { return Ok(py_none()); });
13651357
}
13661358

13671359
PyResult<PyObject *> write(PyTuple *args, PyDict *kwargs)
@@ -1393,24 +1385,29 @@ class BufferedWriter
13931385
}
13941386

13951387
// flush our internal buffer to raw
1396-
if (auto r = raw_write(); r.is_err()) { return r; }
1388+
if (auto r = flush(); r.is_err()) { return r; }
13971389

13981390
// TODO: this currently writes to internal buffer and then immediately makes a copy in
13991391
// write_raw. This is because streambuf doesn't tell us how much capacity it has left, so we
14001392
// just give it as much as we can each time
14011393
ssize_t remaining = buffer.len;
14021394
char *start = static_cast<char *>(buffer.buf->get_buffer());
1403-
while (remaining > written) {
1404-
const auto count = this->buffer->sputn(start + written, buffer.len);
1395+
// TODO: use a buffer that actually tells us how much we can write to it, rather than
1396+
// hardcode 4096
1397+
while (remaining > 4096) {
14051398
// flush our internal buffer to raw
1406-
if (auto r = raw_write(); r.is_err()) { return r; }
1399+
auto r = raw_write(start + written, remaining);
1400+
if (r.is_err()) { return r; }
1401+
ASSERT(as<PyInteger>(r.unwrap()));
1402+
const auto count = as<PyInteger>(r.unwrap())->as_size_t();
14071403
written += count;
14081404
remaining -= count;
14091405
}
14101406

1411-
// now we are done copying everything from buffer to our internal buffer
1412-
// and we also have written it all out to raw (since we don't look up internal buffer
1413-
// capacity before write)
1407+
// put the rest in our buffer. Assumes that we can write 4096 bytes
1408+
const auto count = this->buffer->sputn(start + written, remaining);
1409+
ASSERT(remaining - count == 0);
1410+
14141411
return PyInteger::create(written);
14151412
}
14161413

@@ -1449,11 +1446,27 @@ template<> PyResult<PyObject *> Buffered<BufferedWriter>::flush_and_rewind()
14491446

14501447
PyResult<PyObject *> BufferedWriter::flush()
14511448
{
1452-
if (auto err = check_initialized(); err.is_err()) return Err(err.unwrap_err());
1453-
if (auto err = Buffered<BufferedWriter>::check_closed(""); err.is_err()) {
1454-
return Err(err.unwrap_err());
1449+
if (!writable_) { return Ok(py_none()); }
1450+
std::array<char, BUFSIZ> to_write;
1451+
1452+
while (true) {
1453+
// very innefficient - we shouldn't be getting data from buffer, create an intermediate
1454+
// bytes buffer copy just to then get a view of it.
1455+
auto written = buffer->sgetn(to_write.data(), BUFSIZ);
1456+
if (written <= 0) { break; }
1457+
auto *raw_bytes = bit_cast<std::byte *>(to_write.data());
1458+
auto bytes = PyBytes::create(Bytes{ .b = { raw_bytes, raw_bytes + written } });
1459+
auto memobj = PyMemoryView::create(bytes.unwrap());
1460+
if (memobj.is_err()) { return memobj; }
1461+
if (auto r = raw->get_method(PyString::create("write").unwrap())
1462+
.and_then([memobj](PyObject *write) {
1463+
return write->call(PyTuple::create(memobj.unwrap()).unwrap(), nullptr);
1464+
});
1465+
r.is_err()) {
1466+
return r;
1467+
}
14551468
}
1456-
return raw_write().and_then([](auto) { return Ok(py_none()); });
1469+
return Ok(py_none());
14571470
}
14581471

14591472
template<> BufferedReader *as(PyObject *obj)
@@ -2166,7 +2179,7 @@ class FileIO : public RawIOBase
21662179
.def("readall", &FileIO::readall)
21672180
.def("write", &FileIO::write)
21682181
.def("close", &FileIO::close)
2169-
.def("close", &FileIO::flush)
2182+
.def("flush", &FileIO::flush)
21702183
.finalize();
21712184
}
21722185
module->add_symbol(PyString::create("FileIO").unwrap(), s_io_fileio);

0 commit comments

Comments
 (0)