tqchen commented on code in PR #78:
URL: https://github.com/apache/tvm-ffi/pull/78#discussion_r2404460120
##########
src/ffi/extra/json_writer.cc:
##########
@@ -34,14 +34,15 @@
#include <cstdint>
#include <limits>
#include <string>
+#include <utility>
namespace tvm {
namespace ffi {
namespace json {
class JSONWriter {
public:
- static String Stringify(const json::Value& value, Optional<int> indent) {
+ static String Stringify(const json::Value& value, const Optional<int>&
indent) {
Review Comment:
optional of pod type can be passed by value
##########
include/tvm/ffi/function.h:
##########
@@ -711,11 +713,11 @@ class TypedFunction<R(Args...)> {
* \param args The arguments
* \returns The return value.
*/
- TVM_FFI_INLINE R operator()(Args... args) const {
+ TVM_FFI_INLINE R operator()(const Args&... args) const {
Review Comment:
I am not sure const reference here is better than std::forward, likely
keeping original approach is better
##########
src/ffi/extra/testing.cc:
##########
@@ -235,7 +236,7 @@ TVM_FFI_STATIC_INIT_BLOCK() {
}
std::this_thread::sleep_for(std::chrono::seconds(1));
}
- std::cout << "Function finished without catching signal" <<
std::endl;
+ std::cout << "Function finished without catching signal" << '\n';
Review Comment:
std::endl is needed for flush
##########
include/tvm/ffi/object.h:
##########
@@ -202,10 +202,7 @@ class Object {
TVMFFIObject header_;
public:
- Object() {
- header_.combined_ref_count = 0;
- header_.deleter = nullptr;
- }
+ Object() : header_{} {}
Review Comment:
readability, direct blanket intiialization is not as readable, even perhaps
less efficient than assign
##########
src/ffi/extra/library_module_dynamic_lib.cc:
##########
@@ -45,7 +45,7 @@ namespace ffi {
class DSOLibrary final : public Library {
public:
explicit DSOLibrary(const String& name) { Load(name); }
- ~DSOLibrary() {
+ ~DSOLibrary() override {
Review Comment:
virtual destructor do not need override keyword?
##########
src/ffi/extra/json_writer.cc:
##########
@@ -187,7 +188,7 @@ class JSONWriter {
if (indent_ != 0) {
total_indent_ += indent_;
}
- for (size_t i = 0; i < value.size(); ++i) {
+ for (int64_t i = 0, n = static_cast<int64_t>(value.size()); i < n; ++i) {
Review Comment:
likely original code is better
##########
src/ffi/extra/library_module_system_lib.cc:
##########
@@ -40,7 +41,7 @@ class SystemLibSymbolRegistry {
auto it = symbol_table_.find(name);
if (it != symbol_table_.end() && ptr != (*it).second) {
std::cerr << "Warning:SystemLib symbol " << name << " get overriden to a
different address "
- << ptr << "->" << (*it).second << std::endl;
+ << ptr << "->" << (*it).second << '\n';
Review Comment:
std::endl is better here
##########
include/tvm/ffi/function.h:
##########
@@ -711,11 +713,11 @@ class TypedFunction<R(Args...)> {
* \param args The arguments
* \returns The return value.
*/
- TVM_FFI_INLINE R operator()(Args... args) const {
+ TVM_FFI_INLINE R operator()(const Args&... args) const {
if constexpr (std::is_same_v<R, void>) {
- packed_(std::forward<Args>(args)...);
Review Comment:
likely std::forward is better here via perfect forwarding
##########
src/ffi/backtrace.cc:
##########
@@ -153,7 +153,7 @@ void TVMFFISegFaultHandler(int sig) {
// crashing.
const TVMFFIByteArray* backtrace = TVMFFIBacktrace(nullptr, 0, nullptr, 1);
std::cerr << "!!!!!!! Segfault encountered !!!!!!!\n"
- << std::string(backtrace->data, backtrace->size) << std::endl;
+ << std::string(backtrace->data, backtrace->size) << '\n';
Review Comment:
need std::endl for flush
##########
src/ffi/extra/testing.cc:
##########
@@ -159,7 +160,7 @@ class TestUnregisteredObject : public
TestUnregisteredBaseObject {
TestUnregisteredBaseObject);
};
-TVM_FFI_NO_INLINE void TestRaiseError(String kind, String msg) {
+TVM_FFI_NO_INLINE void TestRaiseError(const String& kind, const String& msg) {
Review Comment:
for the ffi registered functions, given we already ask for a fresh value
that can be moved, pass by value is better, and allows small string be passed
by stack
##########
src/ffi/extra/structural_equal.cc:
##########
@@ -255,7 +255,7 @@ class StructEqualHandler {
}
}
- bool CompareMap(Map<Any, Any> lhs, Map<Any, Any> rhs) {
+ bool CompareMap(const Map<Any, Any>& lhs, const Map<Any, Any>& rhs) {
Review Comment:
under this paticular context, non-const ref is better, mainly because caller
always move into the
##########
src/ffi/backtrace.cc:
##########
@@ -153,7 +153,7 @@ void TVMFFISegFaultHandler(int sig) {
// crashing.
const TVMFFIByteArray* backtrace = TVMFFIBacktrace(nullptr, 0, nullptr, 1);
std::cerr << "!!!!!!! Segfault encountered !!!!!!!\n"
- << std::string(backtrace->data, backtrace->size) << std::endl;
+ << std::string(backtrace->data, backtrace->size) << '\n';
Review Comment:
need `std::endl` to trigger a flush and show in stream
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
