Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Build] Refactor more code to adhere to stricter build warnings #444

Open
wants to merge 1 commit into
base: multiprocess_cc_warnings
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions source/neuropod/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,9 @@
# limitations under the License.

load("@bazel_tools//tools/build_defs/pkg:pkg.bzl", "pkg_tar")
load("//bazel:cc.bzl", "neuropod_cc_binary", "neuropod_cc_library")

cc_library(
neuropod_cc_library(
name = "neuropod_hdrs",
hdrs = [
"neuropod.hh",
Expand All @@ -31,15 +32,15 @@ cc_library(
],
)

cc_library(
neuropod_cc_library(
name = "options",
hdrs = ["options.hh"],
visibility = [
"//visibility:public",
],
)

cc_binary(
neuropod_cc_binary(
name = "libneuropod.so",
linkopts = select({
"@bazel_tools//src/conditions:darwin": ["-Wl,-rpath,@loader_path"],
Expand All @@ -55,7 +56,7 @@ cc_binary(
],
)

cc_library(
neuropod_cc_library(
name = "neuropod_impl",
srcs = [
"neuropod.cc",
Expand Down
6 changes: 4 additions & 2 deletions source/neuropod/bindings/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,9 @@
# See the License for the specific language governing permissions and
# limitations under the License.

cc_binary(
load("//bazel:cc.bzl", "neuropod_cc_binary", "neuropod_cc_library")

neuropod_cc_binary(
name = "neuropod_native.so",
srcs = [
"neuropod_native.cc",
Expand All @@ -31,7 +33,7 @@ cc_binary(
],
)

cc_library(
neuropod_cc_library(
name = "bindings",
srcs = [
"python_bindings.cc",
Expand Down
4 changes: 3 additions & 1 deletion source/neuropod/bindings/c/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,9 @@
# See the License for the specific language governing permissions and
# limitations under the License.

cc_library(
load("//bazel:cc.bzl", "neuropod_cc_library")

neuropod_cc_library(
name = "c_api",
srcs = glob(["*.cc"]),
hdrs = glob(["*.h"]),
Expand Down
8 changes: 4 additions & 4 deletions source/neuropod/bindings/c/c_api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,10 @@ limitations under the License.
#include <vector>

// NOLINTNEXTLINE(readability-identifier-naming): Ignore function case for C API methods
void NP_LoadNeuropodWithOpts(const char * neuropod_path,
const NP_RuntimeOptions *options,
NP_Neuropod ** model,
NP_Status * status)
void NP_LoadNeuropodWithOpts(const char *neuropod_path,
const NP_RuntimeOptions * /*unused*/,
NP_Neuropod **model,
NP_Status * status)
{
try
{
Expand Down
9 changes: 7 additions & 2 deletions source/neuropod/bindings/java/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
# limitations under the License.

load(":java_build_defs.bzl", "JAVACOPTS")
load("//bazel:cc.bzl", "neuropod_cc_binary", "neuropod_cc_library")

genrule(
name = "copy_link_jni_md_header",
Expand Down Expand Up @@ -102,7 +103,7 @@ java_library(
visibility = ["//visibility:public"],
)

cc_library(
neuropod_cc_library(
name = "copy_jni_hdr_lib",
hdrs = [
":copy_link_jni_header",
Expand All @@ -111,11 +112,15 @@ cc_library(
includes = ["."],
)

cc_binary(
neuropod_cc_binary(
name = "libneuropod_jni.so",
srcs = glob(["src/main/native/*.cc"]) + glob(["src/main/native/*.h"]) + [
"//neuropod:libneuropod.so",
],
copts = [
# The autogenerated headers for JNI use reserved names
"-Wno-reserved-id-macro",
],
includes = select({
"@bazel_tools//src/conditions:darwin": ["external/local_jdk/include/darwin"],
"//conditions:default": ["external/local_jdk/include/linux"],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ JNIEXPORT jlong JNICALL Java_com_uber_neuropod_Neuropod_nativeNew__Ljava_lang_St
}

// NOLINTNEXTLINE(readability-identifier-naming): Ignore function case for Java API methods
JNIEXPORT void JNICALL Java_com_uber_neuropod_Neuropod_nativeDelete(JNIEnv *env, jobject obj, jlong handle)
JNIEXPORT void JNICALL Java_com_uber_neuropod_Neuropod_nativeDelete(JNIEnv *env, jobject /*unused*/, jlong handle)
{
try
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,12 +75,12 @@ JNIEXPORT jlongArray JNICALL Java_com_uber_neuropod_NeuropodTensor_nativeGetDims
{
auto tensor = (*reinterpret_cast<std::shared_ptr<neuropod::NeuropodValue> *>(handle))->as_tensor();
auto dims = tensor->as_tensor()->get_dims();
jlongArray result = env->NewLongArray(dims.size());
jlongArray result = env->NewLongArray(static_cast<jsize>(dims.size()));
if (!result)
{
throw std::runtime_error("out of memory");
}
env->SetLongArrayRegion(result, 0, dims.size(), reinterpret_cast<jlong *>(dims.data()));
env->SetLongArrayRegion(result, 0, static_cast<jsize>(dims.size()), reinterpret_cast<jlong *>(dims.data()));
return result;
}
catch (const std::exception &e)
Expand Down Expand Up @@ -116,7 +116,7 @@ JNIEXPORT jlong JNICALL Java_com_uber_neuropod_NeuropodTensor_nativeGetNumberOfE
try
{
auto tensor = (*reinterpret_cast<std::shared_ptr<neuropod::NeuropodValue> *>(handle))->as_tensor();
return tensor->get_num_elements();
return static_cast<jlong>(tensor->get_num_elements());
}
catch (const std::exception &e)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ JNIEXPORT jlong JNICALL Java_com_uber_neuropod_NeuropodTensorAllocator_nativeAll
// Prepare Buffer
auto globalBufferRef = env->NewGlobalRef(buffer);
auto bufferAddress = env->GetDirectBufferAddress(buffer);
const neuropod::Deleter deleter = [globalBufferRef, env](void *unused) mutable {
const neuropod::Deleter deleter = [globalBufferRef, env](void * /*unused*/) mutable {
env->DeleteGlobalRef(globalBufferRef);
};
std::shared_ptr<neuropod::NeuropodValue> tensor;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ jmethodID com_uber_neuropod_NeuropodTensor_getHandle;

jclass com_uber_neuropod_NeuropodJNIException;

jint JNI_VERSION = JNI_VERSION_1_8;
constexpr jint JNI_VERSION = JNI_VERSION_1_8;

bool isTestMode = false;

Expand All @@ -68,7 +68,7 @@ using namespace neuropod::jni;

// This function is called when the JNI is loaded.
// NOLINTNEXTLINE(readability-identifier-naming): Ignore function case for Java API methods
jint JNI_OnLoad(JavaVM *vm, void *reserved)
jint JNI_OnLoad(JavaVM *vm, void * /*unused*/)
{
// Obtain the JNIEnv from the VM and confirm JNI_VERSION
JNIEnv *env;
Expand Down Expand Up @@ -131,7 +131,7 @@ jint JNI_OnLoad(JavaVM *vm, void *reserved)

// This function is called when the JNI is unloaded.
// NOLINTNEXTLINE(readability-identifier-naming): Ignore function case for Java API methods
void JNI_OnUnload(JavaVM *vm, void *reserved)
void JNI_OnUnload(JavaVM *vm, void * /*unused*/)
{
// Obtain the JNIEnv from the VM
JNIEnv *env;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,4 +52,4 @@ extern jclass com_uber_neuropod_NeuropodJNIException;

extern bool isTestMode;
} // namespace jni
} // namespace neuropod
} // namespace neuropod
4 changes: 3 additions & 1 deletion source/neuropod/bindings/neuropod_native.cc
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,9 @@ RuntimeOptions get_options_from_kwargs(py::kwargs &kwargs)
{
RuntimeOptions options;

for (const auto &item : kwargs)
// Note: 'item' is always a copy because the range of type 'py::kwargs' does not return a reference
// Explicitly not using a reference type
for (const auto item : kwargs)
{
const auto key = item.first.cast<std::string>();
const auto &value = item.second;
Expand Down
12 changes: 6 additions & 6 deletions source/neuropod/bindings/python_bindings.cc
Original file line number Diff line number Diff line change
Expand Up @@ -73,16 +73,16 @@ std::shared_ptr<NeuropodTensor> tensor_from_string_numpy(NeuropodTensorAllocator
std::vector<int64_t> & shape)
{
// Unfortunately, for strings, we need to copy all the data in the tensor
auto tensor = allocator.allocate_tensor<std::string>(shape);
int max_len = array.itemsize();
int numel = tensor->get_num_elements();
auto tensor = allocator.allocate_tensor<std::string>(shape);
auto max_len = static_cast<size_t>(array.itemsize());
size_t numel = tensor->get_num_elements();

// Get a pointer to the underlying data
char *data = static_cast<char *>(array.mutable_data());

std::vector<std::string> out;
std::string chars_to_strip("\0", 1);
for (int i = 0; i < numel * max_len; i += max_len)
for (size_t i = 0; i < numel * max_len; i += max_len)
{
std::string item(data + i, max_len);

Expand Down Expand Up @@ -120,7 +120,7 @@ std::shared_ptr<NeuropodTensor> tensor_from_numpy(NeuropodTensorAllocator &alloc
// Capture the array in our deleter so it doesn't get deallocated
// until we're done
auto to_delete = std::make_shared<py::array>(array);
auto deleter = [to_delete](void *unused) mutable {
auto deleter = [to_delete](void * /*unused*/) mutable {
py::gil_scoped_acquire gil;
to_delete.reset();
};
Expand Down Expand Up @@ -169,7 +169,7 @@ py::array tensor_to_numpy(std::shared_ptr<NeuropodTensor> value)
auto data = internal::NeuropodTensorRawDataAccess::get_untyped_data_ptr(*tensor);

// Make sure we don't deallocate the tensor until the numpy array is deallocated
auto deleter = [value](void *unused) {};
auto deleter = [value](void * /*unused*/) {};
auto deleter_handle = register_deleter(deleter, nullptr);
auto capsule = py::capsule(deleter_handle, [](void *handle) { run_deleter(handle); });
return py::array(get_py_type(*tensor), dims, data, capsule);
Expand Down
4 changes: 3 additions & 1 deletion source/neuropod/conversions/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,9 @@
# See the License for the specific language governing permissions and
# limitations under the License.

cc_library(
load("//bazel:cc.bzl", "neuropod_cc_library")

neuropod_cc_library(
name = "eigen",
hdrs = [
"eigen.hh",
Expand Down
5 changes: 3 additions & 2 deletions source/neuropod/core/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,9 @@
# limitations under the License.

load("@bazel_tools//tools/build_defs/pkg:pkg.bzl", "pkg_tar")
load("//bazel:cc.bzl", "neuropod_cc_library")

cc_library(
neuropod_cc_library(
name = "core",
hdrs = glob(["*.hh"]),
visibility = [
Expand All @@ -26,7 +27,7 @@ cc_library(
],
)

cc_library(
neuropod_cc_library(
name = "impl",
srcs = glob(["*.cc"]),
visibility = [
Expand Down
2 changes: 1 addition & 1 deletion source/neuropod/neuropod.cc
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ Neuropod::Neuropod(const std::string & neuropod_path,
}

// Load the model config and use the backend that was provided by the user
Neuropod::Neuropod(const std::string &neuropod_path, std::shared_ptr<NeuropodBackend> backend)
Neuropod::Neuropod(const std::string & /*unused*/, std::shared_ptr<NeuropodBackend> backend)
: backend_(std::move(backend))
{
}
Expand Down
5 changes: 3 additions & 2 deletions source/neuropod/serialization/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,9 @@
# limitations under the License.

load("@bazel_tools//tools/build_defs/pkg:pkg.bzl", "pkg_tar")
load("//bazel:cc.bzl", "neuropod_cc_library")

cc_library(
neuropod_cc_library(
name = "serialization",
hdrs = [
"serialization.hh",
Expand All @@ -27,7 +28,7 @@ cc_library(
],
)

cc_library(
neuropod_cc_library(
name = "impl",
srcs = [
"serialization.cc",
Expand Down
2 changes: 1 addition & 1 deletion source/neuropod/serialization/serialization.cc
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ std::shared_ptr<NeuropodValue> deserialize(boost::archive::binary_iarchive &ar,

void serialize(boost::archive::binary_oarchive &out, const NeuropodValueMap &item)
{
int num_items = item.size();
auto num_items = static_cast<int>(item.size());
Copy link
Contributor

@vkuzmin-uber vkuzmin-uber Sep 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it supposed to be int here? Or this is for backward compatibility with existing models? Maybe "unsigned int" can be used in this case (but need to keep it in sync with deserialize?

Anyway, if you decide to use int, consider using boost::numeric_cast, this is for safe conversion in case like this one

out << num_items;

for (const auto &pair : item)
Expand Down
2 changes: 1 addition & 1 deletion source/neuropod/tests/ope_overrides.hh
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ limitations under the License.
namespace detail
{

std::string get_cwd()
static std::string get_cwd()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

asked already about static in other review

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe anonymous namespace instead of detail will work here instead of "static"

{
char buf[FILENAME_MAX];
getcwd(buf, FILENAME_MAX);
Expand Down