From 34e54383386fb91a827becdabab62c932ed68ce4 Mon Sep 17 00:00:00 2001 From: Guillaume Fraux Date: Wed, 6 Sep 2023 17:30:02 +0200 Subject: [PATCH] Run clang-tidy on all the C++ code --- rascaline-c-api/include/rascaline.hpp | 73 ++++++++++----------- rascaline-torch/include/rascaline/torch.hpp | 6 +- rascaline-torch/src/autograd.cpp | 44 ++++++------- rascaline-torch/src/register.cpp | 15 ++--- 4 files changed, 68 insertions(+), 70 deletions(-) diff --git a/rascaline-c-api/include/rascaline.hpp b/rascaline-c-api/include/rascaline.hpp index a3b2d21a5..afdd575c1 100644 --- a/rascaline-c-api/include/rascaline.hpp +++ b/rascaline-c-api/include/rascaline.hpp @@ -31,8 +31,8 @@ namespace rascaline { class RascalineError : public std::runtime_error { public: /// Create a new error with the given message - RascalineError(std::string message): std::runtime_error(message) {} - ~RascalineError() = default; + RascalineError(const std::string& message): std::runtime_error(message) {} + ~RascalineError() override = default; /// RascalineError is copy-constructible RascalineError(const RascalineError&) = default; @@ -48,7 +48,7 @@ namespace details { /// Class able to store exceptions and retrieve them later class ExceptionsStore { public: - ExceptionsStore(): map_(), next_id_(-1) {} + ExceptionsStore() = default; ExceptionsStore(const ExceptionsStore&) = delete; ExceptionsStore(ExceptionsStore&&) = delete; @@ -58,7 +58,7 @@ namespace details { /// Save an exception pointer inside the exceptions store and return the /// corresponding id as a **negative** integer. int32_t save_exception(std::exception_ptr exception) { - auto id = next_id_; + auto exception_id = next_id_; // this should not underflow, but better safe than sorry if (next_id_ == INT32_MIN) { @@ -66,28 +66,28 @@ namespace details { } next_id_ -= 1; - map_.emplace(id, std::move(exception)); - return id; + map_.emplace(exception_id, std::move(exception)); + return exception_id; } /// Get the exception pointer corresponding to the given exception id. /// The id **MUST** have been generated by a previous call to /// `save_exception`. - std::exception_ptr extract_exception(int32_t id) { - auto it = map_.find(id); - if (it == map_.end()) { + std::exception_ptr extract_exception(int32_t exception_id) { + auto iterator = map_.find(exception_id); + if (iterator == map_.end()) { throw RascalineError("internal error: tried to access a non-existing exception"); } - auto exception = it->second; - map_.erase(it); + auto exception = iterator->second; + map_.erase(iterator); return exception; } private: std::unordered_map map_; - int32_t next_id_; + int32_t next_id_ = -1; }; /// Singleton version of `ExceptionsStore`, protected by a mutex to be safe @@ -99,16 +99,16 @@ namespace details { static int32_t save_exception(std::exception_ptr exception) { const std::lock_guard lock(GlobalExceptionsStore::mutex()); auto& store = GlobalExceptionsStore::instance(); - return store.save_exception(std::move(exception)); + return store.save_exception(exception); } /// Get the exception pointer corresponding to the given exception id. /// The id **MUST** have been generated by a previous call to /// `save_exception`. - static std::exception_ptr extract_exception(int32_t id) { + static std::exception_ptr extract_exception(int32_t exception_id) { const std::lock_guard lock(GlobalExceptionsStore::mutex()); auto& store = GlobalExceptionsStore::instance(); - return store.extract_exception(id); + return store.extract_exception(exception_id); } private: @@ -144,8 +144,10 @@ namespace details { __code__ \ return RASCAL_SUCCESS; \ } catch (...) { \ - auto e = std::current_exception(); \ - return details::GlobalExceptionsStore::save_exception(std::move(e));\ + auto exception = std::current_exception(); \ + return details::GlobalExceptionsStore::save_exception( \ + std::move(exception) \ + ); \ } \ } while (false) @@ -248,7 +250,7 @@ class System { [](const void* self, double* cell) { RASCAL_SYSTEM_CATCH_EXCEPTIONS( auto cpp_cell = reinterpret_cast(self)->cell(); - std::memcpy(cell, &cpp_cell[0][0], 9 * sizeof(double)); + std::memcpy(cell, cpp_cell.data(), 9 * sizeof(double)); ); }, // compute_neighbors @@ -293,12 +295,12 @@ class BasicSystems { /// chemfiles](https://chemfiles.org/chemfiles/latest/formats.html). /// /// @throws RascalineError if chemfiles can not read the file - BasicSystems(std::string path): systems_(nullptr), count_(0) { + BasicSystems(const std::string& path) { details::check_status(rascal_basic_systems_read(path.c_str(), &systems_, &count_)); } - ~BasicSystems() { - details::check_status(rascal_basic_systems_free(systems_, count_)); + ~BasicSystems() noexcept { + rascal_basic_systems_free(systems_, count_); } /// BasicSystems is **NOT** copy-constructible @@ -307,12 +309,12 @@ class BasicSystems { BasicSystems& operator=(const BasicSystems&) = delete; /// BasicSystems is move-constructible - BasicSystems(BasicSystems&& other) { + BasicSystems(BasicSystems&& other) noexcept { *this = std::move(other); } /// BasicSystems can be move-assigned - BasicSystems& operator=(BasicSystems&& other) { + BasicSystems& operator=(BasicSystems&& other) noexcept { this->~BasicSystems(); this->systems_ = nullptr; this->count_ = 0; @@ -398,12 +400,12 @@ class LabelsSelection { } /// LabelsSelection can be move-constructed - LabelsSelection(LabelsSelection&& other): LabelsSelection(std::nullopt, std::nullopt) { + LabelsSelection(LabelsSelection&& other) noexcept : LabelsSelection(std::nullopt, std::nullopt) { *this = std::move(other); } /// LabelsSelection can be move-assigned - LabelsSelection& operator=(LabelsSelection&& other) { + LabelsSelection& operator=(LabelsSelection&& other) noexcept { this->subset_ = std::move(other.subset_); this->predefined_ = std::move(other.predefined_); if (this->subset_) { @@ -569,7 +571,7 @@ class Calculator { } ~Calculator() { - details::check_status(rascal_calculator_free(this->calculator_)); + rascal_calculator_free(this->calculator_); } /// Calculator is **NOT** copy-constructible @@ -578,12 +580,12 @@ class Calculator { Calculator& operator=(const Calculator&) = delete; /// Calculator is move-constructible - Calculator(Calculator&& other) { + Calculator(Calculator&& other) noexcept { *this = std::move(other); } /// Calculator can be move-assigned - Calculator& operator=(Calculator&& other) { + Calculator& operator=(Calculator&& other) noexcept { this->~Calculator(); this->calculator_ = nullptr; @@ -597,7 +599,7 @@ class Calculator { auto buffer = std::vector(32, '\0'); while (true) { auto status = rascal_calculator_name( - calculator_, &buffer[0], buffer.size() + calculator_, buffer.data(), buffer.size() ); if (status != RASCAL_BUFFER_SIZE_ERROR) { @@ -615,7 +617,7 @@ class Calculator { auto buffer = std::vector(256, '\0'); while (true) { auto status = rascal_calculator_parameters( - calculator_, &buffer[0], buffer.size() + calculator_, buffer.data(), buffer.size() ); if (status != RASCAL_BUFFER_SIZE_ERROR) { @@ -741,6 +743,8 @@ class Calculator { /// other function). class Profiler { public: + Profiler() = delete; + /// Enable or disable profiling data collection. By default, data collection /// is disabled. /// @@ -766,11 +770,11 @@ class Profiler { /// @param format in which format should the data be provided. `"table"`, /// `"short_table"` and `"json"` are currently supported /// @returns the current profiling data, in the requested format - static std::string get(std::string format) { + static std::string get(const std::string& format) { auto buffer = std::vector(1024, '\0'); while (true) { auto status = rascal_profiling_get( - format.c_str(), &buffer[0], buffer.size() + format.c_str(), buffer.data(), buffer.size() ); if (status != RASCAL_BUFFER_SIZE_ERROR) { @@ -782,11 +786,6 @@ class Profiler { buffer.resize(buffer.size() * 2, '\0'); } } - -private: - // make the constructor private and undefined since this class only offers - // static functions. - Profiler(); }; } diff --git a/rascaline-torch/include/rascaline/torch.hpp b/rascaline-torch/include/rascaline/torch.hpp index 0bfbabcf1..17ce187dc 100644 --- a/rascaline-torch/include/rascaline/torch.hpp +++ b/rascaline-torch/include/rascaline/torch.hpp @@ -1,10 +1,10 @@ #ifndef RASCALINE_TORCH_HPP #define RASCALINE_TORCH_HPP -#include "rascaline/torch/exports.h" +#include "rascaline/torch/exports.h" // IWYU pragma: export -#include "rascaline/torch/system.hpp" -#include "rascaline/torch/calculator.hpp" +#include "rascaline/torch/system.hpp" // IWYU pragma: export +#include "rascaline/torch/calculator.hpp" // IWYU pragma: export #endif diff --git a/rascaline-torch/src/autograd.cpp b/rascaline-torch/src/autograd.cpp index 91b79151b..9f6af7b9a 100644 --- a/rascaline-torch/src/autograd.cpp +++ b/rascaline-torch/src/autograd.cpp @@ -215,7 +215,7 @@ std::vector PositionsGrad::forward( auto samples = dX_dr->samples(); auto samples_values = samples->values(); - auto samples_values_ptr = samples_values.data_ptr(); + auto* samples_values_ptr = samples_values.data_ptr(); always_assert(samples->names().size() == 3); always_assert(samples->names()[0] == "sample"); always_assert(samples->names()[1] == "structure"); @@ -224,15 +224,15 @@ std::vector PositionsGrad::forward( // ========================= extract pointers =========================== // auto dA_dr = torch::zeros_like(all_positions); always_assert(dA_dr.is_contiguous() && dA_dr.is_cpu()); - auto dA_dr_ptr = dA_dr.data_ptr(); + auto* dA_dr_ptr = dA_dr.data_ptr(); auto dX_dr_values = dX_dr->values(); always_assert(dX_dr_values.is_contiguous() && dX_dr_values.is_cpu()); - auto dX_dr_ptr = dX_dr_values.data_ptr(); + auto* dX_dr_ptr = dX_dr_values.data_ptr(); const auto& dA_dX_sizes = dA_dX.sizes(); always_assert(dA_dX.is_contiguous() && dA_dX.is_cpu()); - auto dA_dX_ptr = dA_dX.data_ptr(); + auto* dA_dX_ptr = dA_dX.data_ptr(); // total size of component + property dimension int64_t n_features = 1; @@ -284,7 +284,7 @@ std::vector PositionsGrad::backward( auto samples = dX_dr->samples(); auto samples_values = samples->values(); - auto samples_values_ptr = samples_values.data_ptr(); + auto* samples_values_ptr = samples_values.data_ptr(); always_assert(samples->names().size() == 3); always_assert(samples->names()[0] == "sample"); always_assert(samples->names()[1] == "structure"); @@ -293,14 +293,14 @@ std::vector PositionsGrad::backward( // ========================= extract pointers =========================== // auto dX_dr_values = dX_dr->values(); always_assert(dX_dr_values.is_contiguous() && dX_dr_values.is_cpu()); - auto dX_dr_ptr = dX_dr_values.data_ptr(); + auto* dX_dr_ptr = dX_dr_values.data_ptr(); always_assert(dB_d_dA_dr.is_contiguous() && dB_d_dA_dr.is_cpu()); - auto dB_d_dA_dr_ptr = dB_d_dA_dr.data_ptr(); + auto* dB_d_dA_dr_ptr = dB_d_dA_dr.data_ptr(); const auto& dA_dX_sizes = dA_dX.sizes(); always_assert(dA_dX.is_contiguous() && dA_dX.is_cpu()); - auto dA_dX_ptr = dA_dX.data_ptr(); + auto* dA_dX_ptr = dA_dX.data_ptr(); // total size of component + property dimension int64_t n_features = 1; @@ -323,7 +323,7 @@ std::vector PositionsGrad::backward( if (dA_dX.requires_grad()) { dB_d_dA_dX = torch::zeros_like(dA_dX); always_assert(dB_d_dA_dX.is_contiguous() && dB_d_dA_dX.is_cpu()); - auto dB_d_dA_dX_ptr = dB_d_dA_dX.data_ptr(); + auto* dB_d_dA_dX_ptr = dB_d_dA_dX.data_ptr(); // dX_dr.shape == [positions gradient samples, 3, features...] // dB_d_dA_dr.shape == [n_atoms, 3] @@ -372,22 +372,22 @@ std::vector CellGrad::forward( always_assert(all_cells.requires_grad()); auto cell_grad = torch::zeros_like(all_cells); always_assert(cell_grad.is_contiguous() && cell_grad.is_cpu()); - auto cell_grad_ptr = cell_grad.data_ptr(); + auto* cell_grad_ptr = cell_grad.data_ptr(); auto samples = dX_dH->samples(); auto samples_values = samples->values(); - auto samples_values_ptr = samples_values.data_ptr(); + auto* samples_values_ptr = samples_values.data_ptr(); always_assert(samples->names().size() == 1); always_assert(samples->names()[0] == "sample"); // ========================= extract pointers =========================== // auto dX_dH_values = dX_dH->values(); always_assert(dX_dH_values.is_contiguous() && dX_dH_values.is_cpu()); - auto dX_dH_ptr = dX_dH_values.data_ptr(); + auto* dX_dH_ptr = dX_dH_values.data_ptr(); const auto& dA_dX_sizes = dA_dX.sizes(); always_assert(dA_dX.is_contiguous() && dA_dX.is_cpu()); - auto dA_dX_ptr = dA_dX.data_ptr(); + auto* dA_dX_ptr = dA_dX.data_ptr(); // total size of component + property dimension int64_t n_features = 1; @@ -399,16 +399,16 @@ std::vector CellGrad::forward( for (int64_t grad_sample_i=0; grad_sample_icount(); grad_sample_i++) { auto sample_i = samples_values_ptr[grad_sample_i]; // we get the structure from the samples of the values - auto structure_i = structures[sample_i].item(); + auto structure_i = static_cast(structures[sample_i].item()); for (int64_t direction_1=0; direction_1<3; direction_1++) { for (int64_t direction_2=0; direction_2<3; direction_2++) { auto dot = 0.0; for (int64_t i=0; i CellGrad::backward( auto samples = dX_dH->samples(); auto samples_values = samples->values(); - auto samples_values_ptr = samples_values.data_ptr(); + auto* samples_values_ptr = samples_values.data_ptr(); always_assert(samples->names().size() == 1); always_assert(samples->names()[0] == "sample"); // ========================= extract pointers =========================== // auto dX_dH_values = dX_dH->values(); always_assert(dX_dH_values.is_contiguous() && dX_dH_values.is_cpu()); - auto dX_dH_ptr = dX_dH_values.data_ptr(); + auto* dX_dH_ptr = dX_dH_values.data_ptr(); always_assert(dB_d_dA_dH.is_contiguous() && dB_d_dA_dH.is_cpu()); - auto dB_d_dA_dH_ptr = dB_d_dA_dH.data_ptr(); + auto* dB_d_dA_dH_ptr = dB_d_dA_dH.data_ptr(); const auto& dA_dX_sizes = dA_dX.sizes(); always_assert(dA_dX.is_contiguous() && dA_dX.is_cpu()); - auto dA_dX_ptr = dA_dX.data_ptr(); + auto* dA_dX_ptr = dA_dX.data_ptr(); // total size of component + property dimension int64_t n_features = 1; @@ -477,14 +477,14 @@ std::vector CellGrad::backward( if (dA_dX.requires_grad()) { dB_d_dA_dX = torch::zeros_like(dA_dX); always_assert(dB_d_dA_dX.is_contiguous() && dB_d_dA_dX.is_cpu()); - auto dB_d_dA_dX_ptr = dB_d_dA_dX.data_ptr(); + auto* dB_d_dA_dX_ptr = dB_d_dA_dX.data_ptr(); // dX_dH.shape == [cell gradient samples, 3, 3, features...] // dB_d_dA_dH.shape == [structures, 3, 3] // dB_d_dA_dX.shape == [samples, features...] for (int64_t grad_sample_i=0; grad_sample_icount(); grad_sample_i++) { auto sample_i = samples_values_ptr[grad_sample_i]; - auto structure_i = structures[sample_i].item(); + auto structure_i = static_cast(structures[sample_i].item()); for (int64_t i=0; i -#include "rascaline/torch/system.hpp" -#include "rascaline/torch/calculator.hpp" +#include "rascaline/torch.hpp" using namespace rascaline_torch; -TORCH_LIBRARY(rascaline, m) { +TORCH_LIBRARY(rascaline, module) { // There is no way to access the docstrings from Python, so we don't bother // setting them to something useful here. - const std::string DOCSTRING = ""; + const std::string DOCSTRING; - m.class_("System") + module.class_("System") .def(torch::init(), DOCSTRING, {torch::arg("species"), torch::arg("positions"), torch::arg("cell")} @@ -22,7 +21,7 @@ TORCH_LIBRARY(rascaline, m) { .def_property("cell", &SystemHolder::get_cell) ; - m.class_("CalculatorOptions") + module.class_("CalculatorOptions") .def(torch::init()) .def_readwrite("gradients", &CalculatorOptionsHolder::gradients) .def_property("selected_keys", @@ -39,7 +38,7 @@ TORCH_LIBRARY(rascaline, m) { ) ; - m.class_("CalculatorHolder") + module.class_("CalculatorHolder") .def(torch::init(), DOCSTRING, {torch::arg("name"), torch::arg("parameters")} @@ -64,7 +63,7 @@ TORCH_LIBRARY(rascaline, m) { }) ; - m.def( + module.def( "register_autograd(" "__torch__.torch.classes.rascaline.System[] systems," "__torch__.torch.classes.metatensor.TensorMap precomputed,"