Some variables are meant to be unused about oclint

[lqi]

This is interesting, I haven't thought about cases like this. Thank you for bringing this up, @kees-jan.

I don't have a recommendation here at the moment, and I would like to hear your suggestions.

One raw idea pops up in my mind is to discard largely used RAII style constructors, like std::lock_guardstd::mutex and boost::mutex::scoped_lock, and at the same time, be able to let users append new identifiers into this list.

Look forward to learning from you.

[kees-jan]

I am not very up-to-date on the state of the art in static code checkers, so I hoped there would be some standard way to deal with this. Absent that, I was also thinking along the lines of a whitelist, allowing some variables to be left unused. Like you say, the user should be able to extend the list.

There's (in my opinion) two important requirements for this whitelist feature to be practical.

1. It should support templates. boost::scoped_lock is typedeffed from boost::unique_lock<boost::mutex>, and you'd want to allow the general case of boost::unique_lock<T> to be unused.
2. Whether a variable is allowed to be unused, should depend on the constructor called. It makes sense for

boost::unique_lock<T> lock(lockable);

to be unused, but that would not be the case for

boost::unique_lock<T> lock(lockable, boost::defer_lock);

or

boost::unique_lock<T> lock(lockable, boost::try_to_lock);

they both should be used, or something is likely wrong

Of course, this is a wishlist for a final solution. Any subset of the above would be a welcome first step 😄

[lqi]

Thank you @kees-jan for your suggestions. I agree with your ideas.

I have spent some time on this issue today, unfortunately I don't think we are ready to have whitelist for the coming release. Instead, I have enabled suppress feature to this rule, as a compromise, you need to add the annotations like

boost::mutex::scoped_lock lock __attribute__((annotate("oclint:suppress[unused local variable]"))) (mut);

to suppress the warning.

But we could definitely plan it in the future release. Based on my current understanding, here are some technical details as a note-

1. Need to have a better understanding on the generic pattern for RAII, and how to refine the essential information from AST context, so that we could compare it with RAII pattern precisely. UnusedLocalVariableRuleTest.cpp has the AST dump of the VarDecl nodes for reference.
2. RuleConfiguration is a key/value structure and currently supports int, double, and string types as values. vector type needs to be added to the support, so that we could maintain a list of the constructors that we would like to skip.
3. Syntactic format for the whitelist. Since we need to use RuleConfiguration, so the flag looks like something like -rc RAII_WHITELIST=std::lock_guard(mutex_type&)? How to define the standard? It doesn't make sure to have another parser inside a rule just to understand this. It may introduces unnecessary complexity and confuses users. I would rather keep it simple, and ask users kindly suppress the warning for now until we have figured out a good way to do this.
4. A collection of well-known RAIIs that need to be suppressed by default. RAIIs like std::lock_guard is a good candidate, but std::ofstream is not.

I will remove the original milestone from this issue, but I will leave this issue open and urge for discussions and suggestions.

[nschum]

lock is in fact used (invisibly) when it goes out of scope to call the destructor.

The approach I would take is: Don't report objects that have an explicit (maybe non-empty?) destructor, or contain members with such a destructor. This would inevitable lead to (probably lots of) false negatives (including the examples @kees-jan points out), but I don't see any other way of reliably detecting truly unused C++ variables.

Ah, the downsides of programming by side effects... :(

But do we need OCLint to implement this warning at all? Does it offer an advantage over clang's -w-unused-variable. Or is it perhaps possible to leverage that flag for OCLint's output?

[lqi]

Don't report objects that have an explicit (maybe non-empty?) destructor, or contain members with such a destructor.

This might be a good starting point, but probably have to deal with some edge cases.

This would inevitable lead to (probably lots of) false negatives

Balancing between false positive and false negatives is always a theme for static code analysis tools. For the record, we always want to pursue more accurate results. However, when I have to choose between false positive and false negative, I might personally be okay with false positive if the tool provides a good approach for me to suppress the false positives.

In users' perspective, I hope the tool could tell me something whenever it feels something fishy, if the tool is wrong, at least, with manual intervene, I can communicate with the tool to teach it stop reporting false positives. However, if something is wrong, but the tool doesn't notify me, then I probably even will never aware of it - this probably will lead me into bigger trouble eventually, which I want to avoid.

the downsides of programming by side effects

And the fun of programming!

clang's -w-unused-variable

This might be a very old rule in OCLint project, maybe it was introduced before clang has the same feature. But I would like to take a look at how clang handles this part, maybe we can leverage it, maybe we can come up with something better.

Thank you a lot, @nschum, it always be good to have discussions with you.

[DennisOSRM]

+1 on this issue. Would be very useful to have this.

[ryuichis]

This is addressed in #249, so I am closing this one.