Cleanup includes#518
Conversation
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…ast.hpp и неиспользуемые include из Result.hpp - Удален #include <bit> из include/rfl/internal/ptr_cast.hpp (не используется в коде) - Удалены неиспользуемые include из include/rfl/Result.hpp: - #include <algorithm> (не используется) - #include <iostream> (не используется) - #include <optional> (не используется)
Этап 2: Базовые типы (средний риск) Удалены неиспользуемые include из файла Tuple.hpp: - <algorithm> - не используется в коде - <bit> - не используется в коде - <cstdint> - не используется в коде - <limits> - не используется в коде - <memory> - не используется в коде - <optional> - не используется в коде Оставлены только необходимые заголовки: - <array> - используется для std::array - <tuple> - используется для std::tuple - <type_traits> - используется для различных type traits - <utility> - используется для std::integer_sequence и других утилит Все остальные файлы (Literal.hpp, Field.hpp, Object.hpp, Variant.hpp) уже имели оптимальный набор include файлов без неиспользуемых зависимостей. Компиляция проекта проверена и проходит успешно.
…вки <string>, <version>, упрощен оператор сравнения spaceship
unused arguments shaded variables
impl 1
Summary of ChangesHello @KostinPavel, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request undertakes a significant refactoring effort focused on optimizing include directives across the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a great cleanup of unused #include directives, which significantly improves build times and code hygiene. The introduction of a .clang-tidy configuration is also a welcome addition for maintaining code quality.
Most importantly, you've added critical error handling to the Avro integration, which fixes several potential crashes and resource leaks. These are excellent and much-needed improvements.
I've found a couple of issues that should be addressed:
- A resource leak in the Avro reader.
- Leftover debugging print statements in the Avro writer.
- A potential resource leak in the Avro writer.
Once these are fixed, this will be a fantastic contribution to the project. Great work!
| avro_value_decref(&root); | ||
| return error(std::string("Could not read root value: ") + avro_strerror()); |
There was a problem hiding this comment.
This error path is missing a call to avro_reader_free(avro_reader), which will result in a resource leak if avro_generic_value_new fails.
avro_reader_free(avro_reader);
avro_value_decref(&root);
return error(std::string("Could not read root value: ") + avro_strerror());| void set_value(const T& _var, avro_value_t* _val) const { | ||
| std::cout << "[DEBUG] set_value called with type: " << typeid(T).name() << std::endl; | ||
|
|
||
| if constexpr (std::is_same<std::remove_cvref_t<T>, std::string>()) { | ||
| std::cout << "[DEBUG] Processing std::string: " << _var << std::endl; | ||
| int result = avro_value_set_string_len(_val, _var.c_str(), _var.size() + 1); | ||
| if (result != 0) { | ||
| std::cout << "[DEBUG] avro_value_set_string_len failed with result: " << result << std::endl; | ||
| throw std::runtime_error(std::string(__FUNCTION__) + " error("+ std::to_string(result)+"): " + avro_strerror()); | ||
| } | ||
| } else if constexpr (std::is_same<std::remove_cvref_t<T>, | ||
| rfl::Bytestring>() || | ||
| std::is_same<std::remove_cvref_t<T>, | ||
| rfl::Vectorstring>()) { | ||
| auto var = _var; | ||
| avro_value_set_bytes(_val, var.data(), var.size()); | ||
|
|
||
| if (!var.data()) { | ||
| std::cout << "[DEBUG] Bytestring/Vectorstring has no data" << std::endl; | ||
| return; | ||
| } | ||
| std::cout << "[DEBUG] Processing Bytestring/Vectorstring of size: " << var.size() << std::endl; | ||
| int result = avro_value_set_bytes(_val, var.data(), var.size()); | ||
| if (result != 0) { | ||
| std::cout << "[DEBUG] avro_value_set_bytes failed with result: " << result << std::endl; | ||
| throw std::runtime_error(std::string(__FUNCTION__) + " error("+ std::to_string(result)+"): " + avro_strerror()); | ||
| } | ||
| } else if constexpr (std::is_same<std::remove_cvref_t<T>, bool>()) { | ||
| avro_value_set_boolean(_val, _var); | ||
|
|
||
| std::cout << "[DEBUG] Processing bool: " << _var << std::endl; | ||
| int result = avro_value_set_boolean(_val, _var); | ||
| if (result != 0) { | ||
| std::cout << "[DEBUG] avro_value_set_boolean failed with result: " << result << std::endl; | ||
| throw std::runtime_error(std::string(__FUNCTION__) + " error("+ std::to_string(result)+"): " + avro_strerror()); | ||
| } | ||
| } else if constexpr (std::is_floating_point<std::remove_cvref_t<T>>()) { | ||
| avro_value_set_double(_val, static_cast<double>(_var)); | ||
|
|
||
| std::cout << "[DEBUG] Processing floating point: " << _var << std::endl; | ||
| int result = avro_value_set_double(_val, static_cast<double>(_var)); | ||
| if (result != 0) { | ||
| std::cout << "[DEBUG] avro_value_set_double failed with result: " << result << std::endl; | ||
| throw std::runtime_error(std::string(__FUNCTION__) + " error("+ std::to_string(result)+"): " + avro_strerror()); | ||
| } | ||
| } else if constexpr (std::is_integral<std::remove_cvref_t<T>>()) { | ||
| avro_value_set_long(_val, static_cast<std::int64_t>(_var)); | ||
|
|
||
| std::cout << "[DEBUG] Processing integral: " << _var << std::endl; | ||
| int result = avro_value_set_long(_val, static_cast<std::int64_t>(_var)); | ||
| if (result != 0) { | ||
| std::cout << "[DEBUG] avro_value_set_long failed with result: " << result << std::endl; | ||
| throw std::runtime_error(std::string(__FUNCTION__) + " error("+ std::to_string(result)+"): " + avro_strerror()); | ||
| } | ||
| } else if constexpr (internal::is_literal_v<T>) { | ||
| avro_value_set_enum(_val, static_cast<int>(_var.value())); | ||
|
|
||
| std::cout << "[DEBUG] Processing literal enum: " << static_cast<int>(_var.value()) << std::endl; | ||
| int result = avro_value_set_enum(_val, static_cast<int>(_var.value())); | ||
| if (result != 0) { | ||
| std::cout << "[DEBUG] avro_value_set_enum failed with result: " << result << std::endl; | ||
| throw std::runtime_error(std::string(__FUNCTION__) + " error("+ std::to_string(result)+"): " + avro_strerror()); | ||
| } | ||
| } else if constexpr (internal::is_validator_v<T>) { | ||
| std::cout << "[DEBUG] Processing validator type" << std::endl; | ||
| // Для валидаторов используем внутреннее значение | ||
| using ValueType = std::remove_cvref_t<typename T::ReflectionType>; | ||
| const auto val = _var.value(); | ||
| set_value<ValueType>(val, _val); | ||
| } else if constexpr (std::is_same<std::remove_cvref_t<T>, rfl::Timestamp<"%Y-%m-%d">>()) { | ||
| std::cout << "[DEBUG] Processing Timestamp" << std::endl; | ||
| // Конвертируем Timestamp в строку | ||
| const auto str = _var.to_string(); | ||
| set_value<std::string>(str, _val); | ||
| } else if constexpr (std::is_same<std::remove_cvref_t<T>, rfl::Email>()) { | ||
| std::cout << "[DEBUG] Processing Email" << std::endl; | ||
| // Email является паттерном на базе std::string, используем внутреннее значение | ||
| const auto& str = static_cast<const std::string&>(_var); | ||
| set_value<std::string>(str, _val); | ||
| } else { | ||
| std::cout << "[DEBUG] Unsupported type reached: " << typeid(T).name() << std::endl; | ||
| static_assert(rfl::always_false_v<T>, "Unsupported type."); | ||
| } | ||
| } |
There was a problem hiding this comment.
This function contains many std::cout statements for debugging. These should be removed from production code to avoid performance overhead and log spam.
| const auto writer = Writer(&root); | ||
| int result = avro_generic_value_new(_schema.iface(), &root); | ||
| if (result != 0) { | ||
| throw std::runtime_error(std::string(__FUNCTION__) + " error("+ std::to_string(result)+"): " + avro_strerror()); |
There was a problem hiding this comment.
To be consistent with read.hpp and to avoid a potential resource leak, you should call avro_value_decref(&root) here before throwing the exception.
avro_value_decref(&root);
throw std::runtime_error(std::string(__FUNCTION__) + " error("+ std::to_string(result)+"): " + avro_strerror());1 participant
No description provided.