svlint icon indicating copy to clipboard operation
svlint copied to clipboard

style_directives checks in comments too?

Open Timmmm opened this issue 6 months ago • 2 comments

If you have this code

// `ifdef FOO

Then the style_directives lint will complain Remove whitespace preceeding compiler directive. about the ifdef in the comment. That doesn't seem right.

It seems like the rule is purely regex based so it can't be sophisticated enough to detect this. The current regex is

format!("^(.+)`({})", keywords)

This is going to be very prone to false-positives (e.g. markdown in comments for example). I would recommend making it tighter, i.e..

format!("^(\s+)`({})", keywords)

Timmmm avatar Jun 24 '25 14:06 Timmmm

So one things first: A TextRule will never be able to handle all cases (for instance /* */ comments, comment symbols within strings, or within other compiler directives) without re-implementing a parser, which is not the point. The proper tool would be an equivalent to a SyntaxRule that applies before the pre-processor.

Now, your proposed regex solves your use case, but creates other false-negative. For instance, this piece of code (from this rule's testcase) would not fail even though multiple style directives are not at the start of the line:

module `ifdef FOO Foo `else Bar `endif
  (); // ifdef, else, and endif are on a single line.
endmodule

I created a PR with my proposed solution: I look in the string before the directive for a //. If I find one, I assume the directive is commented out and therefore the rule passes.

I checked for your use case, the existing testcases as well as a couple I added and all are correct. As I said not all edge cases are handled, but those I can think about should be niche enough.

Does that work for you ?

skjdbg avatar Sep 10 '25 09:09 skjdbg

Yeah that seems reasonable.

Timmmm avatar Sep 10 '25 09:09 Timmmm