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

Static analysis issues #3204

Closed
ghost opened this issue Nov 25, 2022 · 3 comments
Closed

Static analysis issues #3204

ghost opened this issue Nov 25, 2022 · 3 comments

Comments

@ghost
Copy link

ghost commented Nov 25, 2022

Hello,
We are using fmt as part of spdlog in one of the projects I am working on. Running Klocwork for static analysis against the project's code, several issues are reported in the library. Although many reported cases are false positives, some of them I believe are fixable. I have also checked and most reported issues are related to code that is not available in the newest version of the spdlog (version 1.11.0) and fmt.
Please check the list below.

  1. void function returns value
    include/spdlog/fmt/bundled/format.h:2448 | operator()()
    Code: VOIDRET | Severity: Error (2) | State: Existing | Status: Analyze | Taxonomy: C and C++ | Owner: unowned

Fix:

...
if (!find<IS_CONSTEXPR>(from, to, Char('}'), p))
{
    handler_.on_text(from, to);
    return ;
}
++p;
...
  1. Class specs_setter defines a copy constructor, but no assignment operator
    include/spdlog/fmt/bundled/format.h:1944 | format.h()
    Code: CWARN.COPY.NOASSIGN | Severity: Review (4) | State: Existing | Status: Analyze | Taxonomy: C and C++ | Owner: unowned
    Fix – removal:
FMT_CONSTEXPR specs_setter(const specs_setter& other)
    : specs_(other.specs_) {}
  1. Class dynamic_specs_handler defines a copy constructor, but no assignment operator
    include/spdlog/fmt/bundled/format.h:2174 | format.h()
    Code: CWARN.COPY.NOASSIGN | Severity: Review (4) | State: Existing | Status: Analyze | Taxonomy: C and C++ | Owner: unowned

Fix – removal:

  FMT_CONSTEXPR dynamic_specs_handler(const dynamic_specs_handler& other)
      : specs_setter<char_type>(other),
        specs_(other.specs_),
        context_(other.context_) {}
  1. Class error_handler defines a copy constructor, but no assignment operator
    include/spdlog/fmt/bundled/core.h:407 | core.h()
    Code: CWARN.COPY.NOASSIGN | Severity: Review (4) | State: Existing | Status: Analyze | Taxonomy: C and C++ | Owner: unowned

Fix – removal:

constexpr error_handler(const error_handler&) = default;

Please, share your opinion with me.

@vitaut
Copy link
Contributor

vitaut commented Nov 25, 2022

Please submit a PR.

@ghost ghost mentioned this issue Nov 28, 2022
@ghost
Copy link
Author

ghost commented Nov 30, 2022

As it came out in the review, I think I should elaborate on the first reported issue. There were 4 issues reported in the same loop:

450: void function returns value
include/spdlog/fmt/bundled/format.h:2482 | format.h()
Code: VOIDRET | Severity: Error (2) | State: Existing | Status: Analyze | Taxonomy: C and C++ | Owner: unowned

if (p == end || *p != '}')
    return handler.on_error("unknown format specifier"); // #450
} else {

479: void function returns value
include/spdlog/fmt/bundled/format.h:2484 | format.h()
Code: VOIDRET | Severity: Error (2) | State: Existing | Status: Analyze | Taxonomy: C and C++ | Owner: unowned

} else {
return handler.on_error("missing '}' in format string"); // #479
}

371: void function returns value
include/spdlog/fmt/bundled/format.h:2468 | format.h()
Code: VOIDRET | Severity: Error (2) | State: Existing | Status: Analyze | Taxonomy: C and C++ | Owner: unowned

++p;
if (p == end) return handler.on_error("invalid format string"); // #371
if (static_cast<char>(*p) == '}') {

414: void function returns value
include/spdlog/fmt/bundled/format.h:2465 | format.h()
Code: VOIDRET | Severity: Error (2) | State: Existing | Status: Analyze | Taxonomy: C and C++ | Owner: unowned

if (*begin != '{' && !find<IS_CONSTEXPR>(begin, end, '{', p))
    return write(begin, end); // #414
write(begin, p);

@vitaut
Copy link
Contributor

vitaut commented Nov 30, 2022

I don't think VOIDRET diagnostic makes a lot of sense. Looks like a bug in the analyzer which I suggest reporting to the vendor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant