cppcheck

cppcheck copied to clipboard

… function

Wont this lead to FNs for functions that don't throw? I would assume you would annotate the function with [[noreturn]] for functions that throw. That's what we do in cppcheck to avoid these FPs.

If a function is not annotated and its definition is not seen, we cannot warn. But that's not really a FN, we are just missing information. On a side note, we are using e.g. isEscapeFunction and noreturn to mean the same thing. This should be streamlined.

If a function is not annotated and its definition is not seen, we cannot warn.

Most definitions are not stored in the header files. Requiring to inline all functions seems unacceptable especially since functions signatures can indicate this behavior.

But that's not really a FN, we are just missing information.

We are not missing information, we are given incorrect information. The function should be annotated with [[noreturn]] and there is even warnings from compilers for this. Perhaps we could add a style check for this..

Even more importantly, how are users supposed to add information here to correctly warn?

void FunctionThatDoesntAlwaysThrow();
const std::string& f(const std::map<std::string, std::string>& dict, const std::string& a) {

    std::map<std::string, std::string>::const_iterator d_it;
    d_it = dict.find(a);
    if (d_it == dict.end()) {
        FunctionThatDoesntAlwaysThrow();
    }
    return d_it->second; // <- true positive
}

We have noexcept for functions that never throw, and we have [[noreturn]] for functions that always throw, but there is no attribute for functions that can throw but not always throw. Usually functions without these annotation are assumed to be able to throw but not always.

This change is unfortunate as there is no way to get better checking even when users are properly annotating their functions(and inlining everything is unacceptable). It seems like we should at least have a flag or setting for this.

On a side note, we are using e.g. isEscapeFunction and noreturn to mean the same thing. This should be streamlined.

isEscapeFunction is set when we analyze the function definition and noreturn is just checking for the attribute.

We have noexcept for functions that never throw, and we have [[noreturn]] for functions that always throw, but there is no attribute for functions that can throw but not always throw. Usually functions without these annotation are assumed to be able to throw but not always.

This is indeed a conundrum. Maybe this could be part of the <noreturn> configuration? noexcept does not imply a returning function though.

This change is unfortunate as there is no way to get better checking even when users are properly annotating their functions(and inlining everything is unacceptable). It seems like we should at least have a flag or setting for this.

You seem to be arguing in favor of "warning because there might be a problem", which is usually not the cppcheck way.

On a side note, we are using e.g. isEscapeFunction and noreturn to mean the same thing. This should be streamlined.

isEscapeFunction is set when we analyze the function definition and noreturn is just checking for the attribute.

Checking for the attribute, or the configuration. Still two names for the same thing. Almost as annoying as the overloaded hasDefault() (default argument vs. C++11 initializer).

This is indeed a conundrum. Maybe this could be part of the configuration?

Do you mean <noreturn> for library configurations? That doesn't seem to apply here.

You seem to be arguing in favor of "warning because there might be a problem", which is usually not the cppcheck way.

We usually try to avoid FPs when the code is incomplete, that is, it is missing header includes which provide the function or class definitions. Adding missing includes(or library configurations) can make it complete, but inlining is not required to make it complete.

We also require the user to be honest with us about the interface. So we do have FPs like this:

struct A {
  int f() const {
    static int n = 1;
    return n++;
  }
};
int g(A a) {
  return a.f() == a.f(); // duplicateExpression
}

This warns about a duplicate expression on both sides even though both functions return a different value. The problem is that the user is lying to us about the method being const.

This is the same issue here. The user has defined ThrowError as a function that returns so we assume the execution continues, which they are not being honest about. The user needs to use noreturn to fix this issue.

Perhaps it is uncommon to use [[noreturn]](I would assume it is common though since there are already compiler warnings for this), but we should at least provide a flag for users that are properly using this so they can get better analysis. This would mean we would need to pass Settings instead of Library to these functions in this case.

We also require the user to be honest with us about the interface.

I think that is reasonable.

Instinctively I feel that it's better with false negatives when we can't know what the function is doing. An option is to write inconclusive warnings with a separate id when there are such function calls. And then we can see in daca@home if the noise ratio is acceptable.

We have noexcept for functions that never throw, and we have [[noreturn]] for functions that always throw, but there is no attribute for functions that can throw but not always throw. Usually functions without these annotation are assumed to be able to throw but not always.

Yes we can require that [[noreturn]] and noexcept are used properly. If those annotations are seen we should trust them in my opinion. But the scenario you say doesn't have a proper annotation.

Unfortunately that [[noreturn]] tweak is only available for C++ users though. Not sure what we can ask our C users to do.

Oh sorry! This is a C++ checker. So C is of course not relevant.

isReturnScope() is also called inforwardanalyzer.cpp, so this probably relevant in other situations as well.

noexcept doesn't really apply here, since you can still call exit() from such a function.

While it is true that there is no way to properly annotate a FunctionThatDoesntAlwaysThrow(),it doesn't really matter, since an always-non-returning function is required to suppress the warning in the original case. I think that the number of cases where we could correctly determine what a potentially-throwing function actually does in a given piece of code (and we would have to see its definition for that) would be very small, so it's not worth trying to handle this in the first place imho.

Our current message already has an either/or (redundant check vs. bad deref), but maybe we could also mention a potential noreturn function somehow, if there is one?

We already have <noreturn>maybe</noreturn> for some reason...

Or should we require that our C++ users use [[noreturn]] / noexcept .. for C users we just bailout and avoid false positives.

Well it would just be for C++11 or greater. However, there are compiler extensions, users for C++98 and C could use as well, which means we should have some kind of settings so the user can inform us if they are using such extensions(or perhaps they can disable it if C++11 user is not using [[noreturn]]). Perhaps something like --assume-noreturn=off?