From 134d1d6c4fa3a78f430976fffcf7280d4ca5bfbd Mon Sep 17 00:00:00 2001 From: helly25 Date: Sat, 20 Jan 2024 11:02:10 +0100 Subject: [PATCH] * Changed `ParseTextProto`, `ParseTextProtoOrDie` to only work for actual derived proto types. * Added `ParseProto` which returns `absl::StatusOr`. * Updated external dependencies. --- .clang-format | 4 +- .gitignore | 1 + .pre-commit-config.yaml | 13 +++++ BUILD.bazel | 27 ++++++++++ CHANGELOG.md | 17 +++++++ WORKSPACE | 4 ++ mbo/proto/BUILD.bazel | 1 + mbo/proto/parse_text_proto.cc | 72 +++++++++++++------------- mbo/proto/parse_text_proto.h | 82 ++++++++++++++++++++---------- mbo/proto/parse_text_proto_test.cc | 19 +++++-- 10 files changed, 175 insertions(+), 65 deletions(-) create mode 100644 .pre-commit-config.yaml create mode 100644 CHANGELOG.md diff --git a/.clang-format b/.clang-format index 66fc91c..8759f11 100644 --- a/.clang-format +++ b/.clang-format @@ -21,18 +21,20 @@ BreakBeforeBinaryOperators: NonAssignment BreakBeforeConceptDeclarations: Always BreakBeforeTernaryOperators: true BreakInheritanceList: BeforeComma +DerivePointerAlignment: false EmptyLineBeforeAccessModifier: Always FixNamespaceComments: true IndentRequiresClause: false InsertBraces: true InsertNewlineAtEOF: true -# InsertTrailingCommas: true <- Would change structs to never bin pack +## InsertTrailingCommas: true <- Would change structs to never bin pack IntegerLiteralSeparator: Binary: 0 Decimal: 3 KeepEmptyLinesAtTheStartOfBlocks: false MaxEmptyLinesToKeep: 1 PackConstructorInitializers: NextLine +PenaltyReturnTypeOnItsOwnLine: 1000 PointerAlignment: Left QualifierAlignment: Left ReferenceAlignment: Left diff --git a/.gitignore b/.gitignore index 3ac771b..f6d0942 100644 --- a/.gitignore +++ b/.gitignore @@ -8,3 +8,4 @@ /compile_commands.json # Ignore the directory in which `clangd` stores its local index. /.cache/ +/.vscode/ diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml new file mode 100644 index 0000000..578f206 --- /dev/null +++ b/.pre-commit-config.yaml @@ -0,0 +1,13 @@ +repos: +- repo: https://github.com/pre-commit/pre-commit-hooks + rev: v2.3.0 + hooks: + - id: check-merge-conflict + - id: check-yaml + - id: end-of-file-fixer + - id: trailing-whitespace + +- repo: https://github.com/pre-commit/mirrors-clang-format + rev: v16.0.4 + hooks: + - id: clang-format diff --git a/BUILD.bazel b/BUILD.bazel index 9585948..a7ada94 100644 --- a/BUILD.bazel +++ b/BUILD.bazel @@ -1,3 +1,30 @@ +# Copyright 2024 M.Boerger +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +load("@hedron_compile_commands//:refresh_compile_commands.bzl", "refresh_compile_commands") + package(default_visibility = ["//visibility:private"]) licenses(["notice"]) + +COM_GOOGLE_PROTOBUF="bazel-out/../../../external/com_google_protobuf/src" + +refresh_compile_commands( + name = "refresh_all", + targets = { + "//...": " ".join([ + "--cxxopt=-I" + COM_GOOGLE_PROTOBUF, + ]) + }, +) diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 0000000..f048b42 --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,17 @@ +# 0.4 + +* Changed `ParseTextProto`, `ParseTextProtoOrDie` to only work for actual derived proto types. +* Added `ParseProto` which returns `absl::StatusOr`. +* Updated external dependencies. + +# 0.3 + +* Added missing dependency. + +# 0.2 + +* Small fixes, mostly to support a few older abseil/re2 versions. + +# 0.1 + +* Initial release \ No newline at end of file diff --git a/WORKSPACE b/WORKSPACE index b44115a..baee23b 100644 --- a/WORKSPACE +++ b/WORKSPACE @@ -18,6 +18,10 @@ load(":workspace.bzl", "workspace_load_modules") workspace_load_modules() +load("@bazel_skylib//:workspace.bzl", "bazel_skylib_workspace") + +bazel_skylib_workspace() + load("@com_google_protobuf//:protobuf_deps.bzl", "protobuf_deps") protobuf_deps() diff --git a/mbo/proto/BUILD.bazel b/mbo/proto/BUILD.bazel index 39cd2bb..7176a34 100644 --- a/mbo/proto/BUILD.bazel +++ b/mbo/proto/BUILD.bazel @@ -54,6 +54,7 @@ cc_library( "@com_google_absl//absl/log:absl_check", "@com_google_absl//absl/log:absl_log", "@com_google_absl//absl/status", + "@com_google_absl//absl/status:statusor", "@com_google_absl//absl/strings:str_format", "@com_google_absl//absl/strings", "@com_google_protobuf//:protobuf", diff --git a/mbo/proto/parse_text_proto.cc b/mbo/proto/parse_text_proto.cc index d0330a7..af864cf 100644 --- a/mbo/proto/parse_text_proto.cc +++ b/mbo/proto/parse_text_proto.cc @@ -25,53 +25,50 @@ #include "absl/strings/str_format.h" #include "absl/strings/string_view.h" -namespace mbo::proto { -namespace internal { + +namespace mbo::proto::internal { namespace { // Collects errors from proto parsing. class SilentErrorCollector : public google::protobuf::io::ErrorCollector { public: struct ErrorInfo { - int line; - int column; + int line = 0; + int column = 0; std::string message; absl::LogSeverity severity = absl::LogSeverity::kError; }; - void AddError(int line, int column, const std::string& message) override; - void AddWarning(int line, int column, const std::string& message) override; + // NOLINTNEXTLINE(bugprone-easily-swappable-parameters) + void AddError(int line, int column, const std::string& message) override { + errors_.push_back({ + .line = line, + .column = column, + .message = message, + .severity = absl::LogSeverity::kError, + }); + } - std::string GetErrors() const; - const std::vector& errors() const { return errors_; } + // NOLINTNEXTLINE(bugprone-easily-swappable-parameters) + void AddWarning(int line, int column, const std::string& message) override { + errors_.push_back({ + .line = line, + .column = column, + .message = message, + .severity = absl::LogSeverity::kWarning, + }); + } + + std::string GetErrorStr() const; + const std::vector& GetErrors() const { return errors_; } private: std::vector errors_; }; -void SilentErrorCollector::AddError(int line, int column, - const std::string& message) { - ErrorInfo error; - error.line = line; - error.column = column; - error.message = message; - error.severity = absl::LogSeverity::kError; - errors_.push_back(error); -} - -void SilentErrorCollector::AddWarning(int line, int column, - const std::string& message) { - ErrorInfo error; - error.line = line; - error.column = column; - error.message = message; - error.severity = absl::LogSeverity::kWarning; - errors_.push_back(error); -} - -std::string SilentErrorCollector::GetErrors() const { +std::string SilentErrorCollector::GetErrorStr() const { std::string result; - for (const auto& error : errors()) { + for (const auto& error : GetErrors()) { absl::StrAppendFormat(&result, "Line %d, Col %d: %s\n", error.line, error.column, error.message); } @@ -80,18 +77,23 @@ std::string SilentErrorCollector::GetErrors() const { } // namespace -absl::Status ParseTextInternal(std::string_view text_proto, ::google::protobuf::Message* message, +absl::Status ParseTextInternal(std::string_view text, ::google::protobuf::Message* message, std::source_location loc) { google::protobuf::TextFormat::Parser parser; SilentErrorCollector error_collector; parser.RecordErrorsTo(&error_collector); - if (parser.ParseFromString(std::string(text_proto), message)) { + if (parser.ParseFromString(std::string(text), message)) { return absl::OkStatus(); } return absl::InvalidArgumentError( - absl::StrFormat("%s\nFile: '%s', Line: %d", error_collector.GetErrors(), + absl::StrFormat("%s\nFile: '%s', Line: %d", error_collector.GetErrorStr(), loc.file_name(), loc.line())); } -} // namespace internal -} // namespace mbo::proto +void ParseTextOrDieInternal(std::string_view text, ::google::protobuf::Message* message, std::string_view func, std::source_location loc) { + absl::Status result = internal::ParseTextInternal(text, message, loc); + ABSL_CHECK_OK(result) << func << "<" << message->GetDescriptor()->name() << ">" + << " @" << loc.file_name() << ":" << loc.line() << ": " << result; +} + +} // namespace mbo::proto::internal diff --git a/mbo/proto/parse_text_proto.h b/mbo/proto/parse_text_proto.h index 4d547e3..e482654 100644 --- a/mbo/proto/parse_text_proto.h +++ b/mbo/proto/parse_text_proto.h @@ -22,55 +22,85 @@ #include "absl/log/absl_check.h" #include "absl/status/status.h" -#include "absl/strings/str_format.h" +#include "absl/status/statusor.h" #include "absl/strings/string_view.h" #include "google/protobuf/message.h" namespace mbo::proto { namespace internal { -absl::Status ParseTextInternal(std::string_view text_proto, ::google::protobuf::Message* message, - std::source_location loc); +void ParseTextOrDieInternal( + std::string_view text, + ::google::protobuf::Message* message, + std::string_view func, + std::source_location loc); -} // namespace internal - -// Parses the text in 'text_proto' into a prot message of type 'T'. -// The function dies if parsing fails. -template -T ParseTextOrDie(std::string_view text_proto, - std::source_location loc = std::source_location::current()) { - T message; - absl::Status result = internal::ParseTextInternal(text_proto, &message, loc); - ABSL_CHECK_OK(result) << "ParseTextOrDie<" << T::GetDescriptor()->name() << ">" - << " @" << loc.file_name() << ":" << loc.line() << ": " << result; - return message; -} +absl::Status ParseTextInternal(std::string_view text, ::google::protobuf::Message *message, std::source_location loc); class ParseTextProtoHelper final { public: - ParseTextProtoHelper(std::string_view text_proto, std::source_location loc) - : text_proto_(text_proto), loc_(loc), parsed_(false) {} + ~ParseTextProtoHelper() noexcept { ABSL_CHECK(parsed_) << "ParseTextProtoOrDie result unused"; } - ~ParseTextProtoHelper() { - ABSL_CHECK(parsed_) << "ParseTextProtoOrDie result unused"; - } + ParseTextProtoHelper(std::string_view text_proto, std::source_location loc) noexcept + : text_proto_(text_proto), loc_(loc) {} + + // This is a purely temporary object... no copy or move may be used. + ParseTextProtoHelper(const ParseTextProtoHelper&) noexcept = delete; + ParseTextProtoHelper& operator=(const ParseTextProtoHelper&) noexcept = delete; + ParseTextProtoHelper(ParseTextProtoHelper&&) noexcept = delete; + ParseTextProtoHelper& operator=(ParseTextProtoHelper&&) noexcept = delete; - template + template + requires(std::derived_from && !std::same_as) operator T() { // NOLINT clangtidy(google-explicit-constructor) parsed_ = true; - return ParseTextOrDie(text_proto_, loc_); + T message; + ParseTextOrDieInternal(text_proto_, &message, "ParseTextProtoOrDie", loc_); + return message; } private: const std::string text_proto_; const std::source_location loc_; - bool parsed_; + bool parsed_{false}; }; -inline ParseTextProtoHelper ParseTextProtoOrDie( +} // namespace internal + +// Parses the text in 'text_proto' into the proto message type requested as return type. +// The function dies if parsing fails. Example: +// +// ``` +// MyProtoType message = ParseTextProtoOrDie(R"pb(field: 42)pb"); +// ``` +inline internal::ParseTextProtoHelper ParseTextProtoOrDie( std::string_view text_proto, std::source_location loc = std::source_location::current()) { - return ParseTextProtoHelper(text_proto, loc); + return {text_proto, loc}; +} + +// Parses the text in 'text_proto' into a proto message of type 'T' and return it wrapped as StatusOr. +// If parsing fails, then an error status will be returned. +template +requires(std::derived_from && !std::same_as) +absl::StatusOr ParseText(std::string_view text_proto, std::source_location loc = std::source_location::current()) { + T message; + absl::Status result = internal::ParseTextInternal(text_proto, &message, loc); + if (!result.ok()) { + return result; + } + return message; +} + +// Parses the text in 'text_proto' into a proto message of type 'T'. +// The function dies if parsing fails. +// Use this function only if the return type cannot be determined automatically. +template +requires(std::derived_from && !std::same_as) +T ParseTextOrDie(std::string_view text_proto, std::source_location loc = std::source_location::current()) { + T message; + internal::ParseTextOrDieInternal(text_proto, &message, "ParseTextOrDie", loc); + return message; } } // namespace mbo::proto diff --git a/mbo/proto/parse_text_proto_test.cc b/mbo/proto/parse_text_proto_test.cc index 92b0954..42a9919 100644 --- a/mbo/proto/parse_text_proto_test.cc +++ b/mbo/proto/parse_text_proto_test.cc @@ -27,13 +27,12 @@ using ::mbo::proto::tests::SimpleMessage; class ParseTextProtoTest : public ::testing::Test {}; -TEST_F(ParseTextProtoTest, ParseOk) { +TEST_F(ParseTextProtoTest, ParseTextOrDiePass) { EXPECT_THAT(ParseTextOrDie(""), EqualsProto("")); EXPECT_THAT(ParseTextOrDie("one: 42"), EqualsProto("one: 42")); - EXPECT_THAT((SimpleMessage)ParseTextProtoOrDie("one: 25"), EqualsProto("one: 25")); } -TEST_F(ParseTextProtoTest, ParseError) { +TEST_F(ParseTextProtoTest, ParseTextOrDieFail) { EXPECT_DEATH( ParseTextOrDie("!!!"), // Using [0-9] in lieu of \\d to be compatible in open source. @@ -43,5 +42,19 @@ TEST_F(ParseTextProtoTest, ParseError) { "INVALID_ARGUMENT: Line 0, Col 0: Expected identifier, got: !"); } +TEST_F(ParseTextProtoTest, ParseTextProtoOrDiePass) { + EXPECT_THAT((SimpleMessage)ParseTextProtoOrDie("one: 25"), EqualsProto("one: 25")); +} + +TEST_F(ParseTextProtoTest, ParseTextProtoOrDieFail) { + EXPECT_DEATH( + SimpleMessage msg = ParseTextProtoOrDie("!!!"), + // Using [0-9] in lieu of \\d to be compatible in open source. + ".*Check failed:.*\n*" + "File: '.*/parse_text_proto.*', Line: [0-9]+.*" + "ParseTextProtoOrDie.*" + "INVALID_ARGUMENT: Line 0, Col 0: Expected identifier, got: !"); +} + } // namespace } // namespace mbo::proto