Community
Participate
Working Groups
Build Identifier: Juno RC3 20120606-2254 To help deal with the fact that Codan is imperfect, it would be highly useful if there were a way to flag codan errors in specific lines of code to be ignored. This could work in one of two ways: 1) Through the use of special comment tags ie, you could put functionWhichRaisesACodanError(arg1, arg2) // [CODAN_IGNORE] 2) Through gui integration Ie, you could right click on a codan error (either on the icon in the column to the left of the code, or in the "Problems" window), and tell it to ignore that error. There would then need to be some way to tell Codan to stop ignoring that error, as well - perhaps the icon for the error would still be there, but "ghosted", and you could right click again to show... or there would be a filter - that would be on by default, but could be toggled off - that would show / hide ignored codan errors Reproducible: Always
I agree with Paul - it should be possible to disable single codan checks with a comment as it is possible in for example checkstyle with a //CHECKSTYLE:OFF comment. There is a way to exclude files from codan via a preference page, but this disables the check for the whole file and not only for a single warning. Another major drawback of this solution is that the excluded file list ist stored in the preference store. If you collaborate with others, the exclusions must be set on every machine. Is there any progress on this topic? Maybe I can provide a patch for this if we could agree on a concept how it should work. What about //CODAN:ON //CODAN:OFF comments? Or a // [CODAN_IGNORE] as Paul proposed?
Created attachment 237559 [details] first approach Attached is a patch with a first approach to disable codan problems with a comment. To suppress all codan problems for a given code block use: //CODAN:OFF and //CODAN:ON To suppress a specific codan problem use: //CODAN:OFF #ProblemId and //CODAN:ON #ProblemId There are still some issues to discuss. 1) The ProblemId is the full qualified name of the checker, for example org.eclipse.cdt.codan.internal.checkers.AssignmentToItselfProblem This identifier is too long to include it as a comment in the code. The current approach checks whether the id ends with the specificed ProblemId, so it is sufficient to use CODAN:OFF #AssignmentToItselfProblem. However, the ID to use for the ignore comment should be mentioned within the error message somehow. 2) I added the filter to org.eclipse.cdt.codan.core.model.AbstractChecker to enable the comment suppression for all codan checks, not only for the AbstractIndexAstChecker. This has the drawback, that every resource has to be parsed twice. I don't know yet if this is a significant drawback in performance. 3) There should be some way to hook in custom comment parsers and/or disable built in parsers. Maybe an extension point should be added and some additional preference page where parsers can be enabled and disabled. Would be great to get some feedback on this!!
I worry about the comments messing up people code. I'm not sure how receptive of that they would be. At the very least, don't use upper case letters in the tags. Another approach would be similar to what the API baselines feature does with the .apifilters file. If we could reference symbol names where we'd like codan errors to be ignored, we could enter them in a similar file.
Suppressing warnings with a code comment is quite common, for example lint uses comments like /*lint -esym(534,cputs,fgets,cprintf) */ to suppress specific warnings. I think the second approach could be a little bit confusing. If there is an expression like "x=x" in the code, you would have to look up a separate file to see why the warning is disabled. This is more direct (with lower case letters): //codan:off #AssignmentToItselfProblem I know what I am doing here... x=x //codan:on On the other hand, the code gets a little bit "polluted" with codan specific comments, but I think that is the lesser evil.
Created attachment 237691 [details] second approach I changed the comments from upper to lower case and added an extension point to hook in custom problem filters. Per default, no problem filter is active. The Comment Problem Filter can be activated with the codan preference page. Looking forward to your feedback.
Hi Andreas. I have used Lint quite a bit and I had no problem with the approach of adding comments in the code. It's a bit like @SuppressWarnings in Java when you think about it. The extension point would also allow other solutions if necessary. I haven't really looked at the code yet but one thing I'm not clear on is how the user can discover how to disable a specific problem? Is there a way to get the problem ID (example AssignmentToItselfProblem) without having the Codan source code handy? Maybe we'd need a quick fix to assist the user in doing that. BTW could you push your patch to Gerrit? http://wiki.eclipse.org/CDT/git#Using_Gerrit_for_CDT I also noticed that there were a few extra cosmetic change that could be left out of the patch (line endings in SuppressProblemsTest.java, line wrapping in CodanPreferencePage.java).
Created attachment 237866 [details] third approach Hi Marc-Andre, thanks for the feedback! I made some changes to the patch and committed it to Gerrit. Regarding the point how the user can discover how to disable a specific problem: One option could be to add a filter id to the checker extension point besides the technical id. This filter id could be included in the error message, for example [Codan001] Assignment to itself 'x = x' codan:off #Codan001 x=x codan:on#Codan001 This would also solve the problem, that only the last segment of the full problem id is currently used. Reusing the (technical) problem id in the comments is not very readable. The Quickfix approach is a good idea. I had a look at the Quickfix API today - AFAIK it is currently not possible to register a general quickfix for all codan problems, a problemId is required by the quickfix extension point.
(In reply to Andreas Muelder from comment #7) First, sorry for lack of response :( > Regarding the point how the user can discover how to disable a specific > problem: > One option could be to add a filter id to the checker extension point > besides the technical id. I think adding an optional filter id could be a good solution. I don't think they should be numbered like 'Codan001' though because any other plugin can contribute more checkers/problems and if they try to use the same naming scheme, it will conflict with new filter ids as checkers get added to CDT. I think they can be more descriptive like codan:off #assignment to itself. If they are more descriptive like that, then I think it's oOK if two problems ids happen to have the same filter id, they can be both ignored. Problems not defining the filter id would not be ignorable. > This filter id could be included in the error > message I'm not sure how that would look in the Problems view but this sounds like an acceptable solution until quick fixes are introduced. > The Quickfix approach is a good idea. I had a look at the Quickfix API today > - AFAIK it is currently not possible to register a general quickfix for all > codan problems, a problemId is required by the quickfix extension point. I still think this is the best solution instead of having the filter id in the error message but this effort could be done in a separate patch.
Although I always recommend for user not to use comments approach, I cannot stop users from wanting it because it seems like a quick solution for a specific problem. The right approach as Doug mentioned is to create per instance filters and store them in .settings. But it looks like we need both. If we do use comments approach it would be per line comments, something like x = x; // @suppress("assignment in condition") because it is a test where text is english error title, this is readable at least not too tool specific (vs. good luck understanding what is -esym(534) when reading the code) on/off is too verbose and usually same error don't spawn multiple lines so I am not sure use cases for this
What about a C++11 attribute recognized by codan? [[codan::suppress("assignment in condition")]] x = x; I'm thinking of this as being somewhat analogous to Java's @SuppressWarning annotation.
(In reply to Nathan Ridge from comment #10) > What about a C++11 attribute recognized by codan? > > [[codan::suppress("assignment in condition")]] > x = x; > > I'm thinking of this as being somewhat analogous to Java's @SuppressWarning > annotation. There is no advantages to this compare to using comment but it limits this to c++ 11 In java it makes a lot more sense because annotation are propagate to bytecode so byte code analysers can use them
(In reply to Elena Laskavaia from comment #11) > (In reply to Nathan Ridge from comment #10) > > What about a C++11 attribute recognized by codan? > > > > [[codan::suppress("assignment in condition")]] > > x = x; > > > > I'm thinking of this as being somewhat analogous to Java's @SuppressWarning > > annotation. > > There is no advantages to this compare to using comment but it limits this > to c++ 11 > In java it makes a lot more sense because annotation are propagate to > bytecode so byte code analysers can use them Well, C++ syntax is propagated to the AST, so AST-based tools (such as compilers) can use them. If compilers and tools like codan can agree on a set of attributes like this, then a single attribute could disable warnings given by multiple tools. You are right that this would only be applicable to C++11, though. Just thinking out loud.
New Gerrit change created: https://git.eclipse.org/r/63815
Gerrit change https://git.eclipse.org/r/63815 was merged to [master]. Commit: http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=05daa126a0b6c09907dbe5f842156ec9d15d9d44
https://wiki.eclipse.org/CDT/User/NewIn90#Suppressions_in_code
Looking at the result, I actually like how this feature has turned out. I've gotten used to using @SuppressWarnings in Java to manage warnings in the code. This feels similar to that, even if it's in the comments. However, I found it very hard to use. There doesn't seem to be an ignore quick fix for errors that would insert the magic comments. Am I missing it somewhere else? I can't imagine we're expecting users to know what the magic incantations are. And if we're using JDT and @SuppressWarnings which does things that way.
Yes quick fix would be awesome for that
(In reply to Elena Laskavaia from comment #17) > Yes quick fix would be awesome for that Agreed. I filed bug 495842 to track this.
Created attachment 271497 [details] Patch for ignoring codan checks on windows Ignoring codan checks does not work on windows systems, patch is attached. Any chance to see this in Oxygen.2?
(In reply to Andreas Muelder from comment #19) > Created attachment 271497 [details] > Patch for ignoring codan checks on windows > > Ignoring codan checks does not work on windows systems, patch is attached. > Any chance to see this in Oxygen.2? Thank you for the patch. This exact change has actually been made already in bug 525438. It will be in Oxygen.2.
Great! Thanks for fixing this.
