Cppcheck coding guideliness for use of auto

[Daniel Marjamäki]

I feel that auto is overused in cppcheck. And would like to have a little discussion about it.

I don't want that we just mechanically replace types with auto whenever possible.

Can we agree on when it is OK to use it.

Example 1: Cast result assigned to variable:

const auto t = static_cast<char>(type);

I am not convinced that auto is a good idea if the typename is short and simple. It does not save any bytes to write auto here. If we just save 2-3 bytes by writing auto I still suggest we should write the real typename. I don't think abbreviations should be overused. Could we agree on a limit.. i.e. if typename is 6 bytes or less then writing the real type name is recommended..?

Example 2:

const auto& excludedPaths = project.guiProject.excludedPaths

It's not obvious what the type is here. Sometimes people will have to grep for excludedPaths to determine what the type is.
The type is std::list<std::string>. If we write that then the code is more explicit. What are your thoughts? In my humble opinion it does not hurt readability to write the full type here.

Example 3:
Replacing this code:

for (std::multimap<std::string, const Function*>::const_iterator it = range.first; it != range.second; ++it) {~~~

With:

for (auto it = range.first; it != range.second; ++it) {

In my opinion we are saving so much code here, details are lost but with such a code cleanup the code is easier to read. Opinions?

[Oliver Stöneberg]

I think all three are valid.

Example 1 is not about saving code but redundancies. The type is clearly specified so there is no need to specify it again. So that should be done.

Example 2 it would help understanding the code but if you are using a proper IDE the tooltip should take care of that.

Example 3 it saves a lot of space and I think this is about the as_const() as well. I think that is a good use of auto and again the IDE tooltip would be good here.

Buit in case of 2 or 3 I could go either way. But even with not using auto the as_const() still should be applied (in range loops it might not use the const_iterator even if you use const& - I think I posted that example at some point).

[Paul Fultz]

The problem with example 1 is the static_cast. I do find this common pattern with AI generated code. It is much cleaner to construct the variable with the correct type instead of using a static_cast, like char t = type or char t{type}. I actually have an addon that checks for these redundant casts:

@cppcheck.checker
def RedundantCast(cfg, data):
    for token in cfg.tokenlist:
        if not token.variable:
            continue
        m = match(token,
                  "%var%@decl ; %var%@assign = static_cast <*>@cast (*) ;")
        if not m:
            continue
        if m.decl.varId != m.assign.varId:
            continue
        if not simpleMatch(token.previous, "auto"):
            if not isTokensEqual(getVariableDecl(m.decl.variable),
                                 getInnerLink(m.cast),
                                 skip='const|volatile|&|&&|*'):
                continue
        cppcheck.reportError(token, "style", "Static cast is redundant.")

For example 2, its just shortening the project.guiProject.excludedPaths name. If you used the long form project.guiProject.excludedPaths variable it wouldn't give you anymore type info than the shortened auto variable.

Of course example 3, I think this is obvious why it is helpful for this case.

Also clangd can show the type of the auto variable which works for almost all editors.

[Daniel Marjamäki]

It is much cleaner to construct the variable with the correct type instead of using a static_cast,

+1

I also see such somewhat related code now and then:

    auto s = std::string{"hello"};

[Daniel Marjamäki]

so as I read it we think that example 2 and example 3 are OK.
I believe that example 1 is discussible. There may be situations where auto is preferable and then there are situations where it would be better to remove the cast and not use auto.

Imho, the downsides of the auto+cast is that it's extra code and it can hide compiler warnings that may point out real bugs.