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.

Sign up for GitHub

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

fix: Static analysis issues #3208

Closed
wants to merge 2 commits into from
Closed

fix: Static analysis issues #3208

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
645f140
include/fmt/core.h: copy constructors removal
Nov 28, 2022
5e4a210
include/fmt/core.h: error handling and return separated
Nov 28, 2022
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
33 changes: 16 additions & 17 deletions include/fmt/core.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -641,7 +641,6 @@ FMT_NORETURN FMT_API void throw_format_error(const char* message);

struct error_handler {
constexpr error_handler() = default;
constexpr error_handler(const error_handler&) = default;

// This function is intentionally not constexpr to give a compile-time error.
FMT_NORETURN void on_error(const char* message) {
Expand Down Expand Up @@ -2218,9 +2217,6 @@ template <typename Char> class specs_setter {
explicit FMT_CONSTEXPR specs_setter(basic_format_specs<Char>& specs)
: specs_(specs) {}

FMT_CONSTEXPR specs_setter(const specs_setter& other)
: specs_(other.specs_) {}

FMT_CONSTEXPR void on_align(align_t align) { specs_.align = align; }
FMT_CONSTEXPR void on_fill(basic_string_view<Char> fill) {
specs_.fill = fill;
Expand Down Expand Up @@ -2255,11 +2251,6 @@ class dynamic_specs_handler
ParseContext& ctx)
: specs_setter<char_type>(specs), specs_(specs), context_(ctx) {}

FMT_CONSTEXPR dynamic_specs_handler(const dynamic_specs_handler& other)
: specs_setter<char_type>(other),
specs_(other.specs_),
context_(other.context_) {}

template <typename Id> FMT_CONSTEXPR void on_dynamic_width(Id arg_id) {
specs_.width_ref = make_arg_ref(arg_id);
}
Expand Down Expand Up @@ -2704,8 +2695,10 @@ FMT_CONSTEXPR FMT_INLINE void parse_format_string(
handler.on_text(begin, p - 1);
begin = p = parse_replacement_field(p - 1, end, handler);
} else if (c == '}') {
if (p == end || *p != '}')
return handler.on_error("unmatched '}' in format string");
if (p == end || *p != '}') {
handler.on_error("unmatched '}' in format string");
return;
}
handler.on_text(begin, p);
begin = ++p;
}
Expand All @@ -2718,11 +2711,15 @@ FMT_CONSTEXPR FMT_INLINE void parse_format_string(
if (from == to) return;
for (;;) {
const Char* p = nullptr;
if (!find<IS_CONSTEXPR>(from, to, Char('}'), p))
return handler_.on_text(from, to);
if (!find<IS_CONSTEXPR>(from, to, Char('}'), p)) {
handler_.on_text(from, to);
return;
Comment on lines +2715 to +2716
Copy link
Contributor

@vitaut vitaut Nov 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

These and similar changes elsewhere look unnecessary, please revert.

Copy link
Author

Choose a reason for hiding this comment

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

I commented on the #3204 reasons for this change and 2 more below.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for providing more context but I still don't see reasons for changing this. Let's revert. The other changes seem fine.

}
++p;
if (p == to || *p != '}')
return handler_.on_error("unmatched '}' in format string");
if (p == to || *p != '}') {
handler_.on_error("unmatched '}' in format string");
return;
}
handler_.on_text(from, p);
from = p + 1;
}
Expand All @@ -2733,8 +2730,10 @@ FMT_CONSTEXPR FMT_INLINE void parse_format_string(
// Doing two passes with memchr (one for '{' and another for '}') is up to
// 2.5x faster than the naive one-pass implementation on big format strings.
const Char* p = begin;
if (*begin != '{' && !find<IS_CONSTEXPR>(begin + 1, end, Char('{'), p))
return write(begin, end);
if (*begin != '{' && !find<IS_CONSTEXPR>(begin + 1, end, Char('{'), p)) {
write(begin, end);
return;
}
write(begin, p);
begin = parse_replacement_field(p, end, handler);
}
Expand Down