false positive with { in comments

[rgerhards]

This is in regard to the "commented out code" query. If there is a curly brace inside a comment (/* advance to { */) this is misdetected as commented out code. It's for sure a border case, but given the fact that before the brace no valid C is given, I think it could be sorted out. It's also pretty hard to work around this type of FP.

Sample in context: https://lgtm.com/projects/g/rsyslog/rsyslog/alerts/?mode=list
Right now, you need to scroll down a bit to see the sample.

[jbj]

FPs for this query come up every once in a while, so maybe it's time to look at making it more conservative. It's currently based on a bunch of regexes as far is I recall. Would it be reasonable to only think of a line ending in { as code if there's a ) (plus optional whitespace) preceding the {?

As for workarounds, would it make sense for you to enclose the { in backticks or some other form of quote? I would find that cleaner to read as a human too.

[rgerhards]

FPs for this query come up every once in a while, so maybe it's time to look at making it more conservative. It's currently based on a bunch of regexes as far is I recall. Would it be reasonable to only think of a line ending in { as code if there's a ) (plus optional whitespace) preceding the {?

It's really a though call I admit. I think that should work in most cases, and false negatives should be really seldom. I would assume that something like

/* if (a==b)
     {
*/

would still be caught, right?

As for workarounds, would it make sense for you to enclose the { in backticks or some other form of quote? I would find that cleaner to read as a human too.

ah, I hadn't thought this would work. Actually '{' would sound best to me. I'll try it and post results. It would probably help to mention such cases on https://lgtm.com/rules/2163190799/ -- if there is a good workaround, I am always happy to apply it, and I guess that's true for most others as well. A FP in static analysis is not really bad, but having no work-around is.

[rgerhards]

Jupp, this works: https://lgtm.com/projects/g/rsyslog/rsyslog/rev/pr-7756dfd5ec075fd1446e95713b580c2e22f1d95e

So I would consider adding this work-around to the doc as an acceptable "fix" for the issue.

[geoffw0]

FPs for this query come up every once in a while, so maybe it's time to look at making it more conservative.

I agree. I also think we should aim to have (much) better tests for this query, it's the kind of thing where it's easy to regress one pattern as you fix another.

[jbj]

I've opened #779 to improve this query and its test. The documentation is apparently versioned in our internal repository, and I've opened a PR there to clarify that quoting the code will suppress the alerts.

I'm overall pleased with the low false positive rate of this query on lgtm.com, and it should become even lower with #779.

[jbj]

I'm closing this issue as I think it's been addressed.