-
Notifications
You must be signed in to change notification settings - Fork 491
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
Some variables are meant to be unused #32
Comments
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. |
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.
to be unused, but that would not be the case for
or
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 😄 |
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
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-
I will remove the original milestone from this issue, but I will leave this issue open and urge for discussions and suggestions. |
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 |
This might be a good starting point, but probably have to deal with some edge cases.
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.
And the fun of programming!
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. |
+1 on this issue. Would be very useful to have this. |
This is addressed in #249, so I am closing this one. |
Consider the following:
When running this through oclint 0.6, it claims (correctly)
At this point, I do not want to disable the rule altogether (it might find some useful errors), but I also don't want to annotate each and every time I lock a mutex (or use a similar RAII style construct).
What is the recommended approach here?
The text was updated successfully, but these errors were encountered: