6

I analyzed some code with cppcheck for errors and code quality. I came across an error which I think is an false positive. The following code example shows the problem (marked with comment).

cppcheck - v 1.89

#include <string>
#include <vector>
#include <iostream>

std::string func() {
    std::vector<char> data{};
    data.push_back('a');
    data.push_back('b');
    data.push_back('c');
    return std::string{ data.data(), data.size() }; // error marked here
    // severity: error
    // line: 12
    // summary: Returning object that points to local variable 'data' that will be invalid when returning.
}

int main() {
    std::string ret{};
    {
        ret = func();
    }
    std::cout << ret << std::endl;
    return 0;
}

If i use () instead of {}, it resolves the error.

EDIT

When I debug the example with () or {}, there is totally no difference. I use Visual Studio 17 with C++14.

Tsyvarev
  • 60,011
  • 17
  • 110
  • 153
skratchi.at
  • 1,151
  • 7
  • 22
  • Your line std::string will invoke the constructor for the string class, it requires () because its a function call. {} is used for data initialization and scoping. – SPlatten Nov 14 '19 at 08:26
  • @SPlatten when I debug the example, there is totally no difference in the behavior. – skratchi.at Nov 14 '19 at 08:30
  • 1
    Cppcheck has been broken for years. @SPlatten It is direct list initialization syntax invoking constructor. – user7860670 Nov 14 '19 at 08:31
  • @VTT would you discourage me to use it in an productive context? – skratchi.at Nov 14 '19 at 08:32
  • Well, using it won't do any harm, but it is not that helpful and struggles to parse modern syntax. I'd recommend PVS studio. – user7860670 Nov 14 '19 at 08:41
  • 4
    If what's between the `{}` can be interpreted as a `std::initializer_list`, you have a problem (which can be fixed by using `()`). Otherwise, using `{}` is fine. When in doubt (for classes that have a `std::initializer_list` constructor), it's typically safer to stick with `()`. Ref. eg. [this answer](https://stackoverflow.com/questions/18222926/why-is-list-initialization-using-curly-braces-better-than-the-alternatives/37228443#37228443). – Sander De Dycker Nov 14 '19 at 08:42
  • 1
    Ie. : I suspect cppcheck just "plays it safe". – Sander De Dycker Nov 14 '19 at 08:44
  • As the answers indicate, it really was a false positive. There is a corresponding [ticket in the bug tracker](https://trac.cppcheck.net/ticket/9473) and it has already been fixed with [this commit](https://github.com/danmar/cppcheck/commit/4ebf54d0902eb930cf54bf7b10214b353450e665). So Cppcheck 1.90 will no longer have this false positive. – versat Nov 27 '19 at 14:26

2 Answers2

4

I think the rule was for before C++11:

{/*..*/} was only use for aggregate initialization before C++11, so T{ data.data(), data.size() } could only store the future dangling pointer thus the diagnostic.

With T(data.data(), data.size() ), it is a regular constructor call, so it actually depends of T constructor, so no diagnostic can be safely be done without inspecting T constructor (and I doubt than cppcheck does inspect) (and BTW it is safe for std::string).

So indeed, it is a false positive.

Jarod42
  • 203,559
  • 14
  • 181
  • 302
2

It's a false-positive as the string ctor makes a copy of the passed char* string.

demeter
  • 92
  • 1