Skip to content

Conversation

@jvsqzj
Copy link
Contributor

@jvsqzj jvsqzj commented Jul 18, 2025

VP Developers have pointed out reported warnings are displayed with a line number which is off by one.

Adding one in this line seems like a quick fix for this, but I'd like to know if there are suggestions of another fix for this.
DFA was originally used for testing, but its now part of the integration into VP development PR checks for DML code, so I think its important for the messages to align with expectations from developers and avoid any confusion when trying to apply corrections for these diagnostics.

@JonatanWaern
Copy link
Contributor

Im not too adverse to adding a fix letting the dfa client output 1-indexed line numbers, however this is not the right place to do it as you are changing the DFA's internal representation to 1-indexed. My suggestion is to add a command-line option to the DFA that makes it add one when printing out the numbers in 'output_errors'.

@anankos-intel
Copy link

Im not too adverse to adding a fix letting the dfa client output 1-indexed line numbers, however this is not the right place to do it as you are changing the DFA's internal representation to 1-indexed. My suggestion is to add a command-line option to the DFA that makes it add one when printing out the numbers in 'output_errors'.

that means that we will need to change the tool invocation?

@jvsqzj
Copy link
Contributor Author

jvsqzj commented Jul 23, 2025

Im not too adverse to adding a fix letting the dfa client output 1-indexed line numbers, however this is not the right place to do it as you are changing the DFA's internal representation to 1-indexed. My suggestion is to add a command-line option to the DFA that makes it add one when printing out the numbers in 'output_errors'.

that means that we will need to change the tool invocation?

I think the best way forward to avoid making this change to the dfa wrapper on your end, is to apply what Jonatan suggests here, but with the opposite logic. That means, the default behavior is 1 indexed, and there is an option to use the Zero index format. I think this makes sense as file lines are not usually displayed as zero indexed in text editors.

@anankos-intel
Copy link

that would be great if we do not need to change anything in how we call the tool

@JonatanWaern
Copy link
Contributor

Im not too adverse to adding a fix letting the dfa client output 1-indexed line numbers, however this is not the right place to do it as you are changing the DFA's internal representation to 1-indexed. My suggestion is to add a command-line option to the DFA that makes it add one when printing out the numbers in 'output_errors'.

that means that we will need to change the tool invocation?

I think the best way forward to avoid making this change to the dfa wrapper on your end, is to apply what Jonatan suggests here, but with the opposite logic. That means, the default behavior is 1 indexed, and there is an option to use the Zero index format. I think this makes sense as file lines are not usually displayed as zero indexed in text editors.

Yes this is also fine.

@alecalvop
Copy link
Contributor

@JonatanWaern

My suggestion is to add a command-line option to the DFA that makes it add one when printing out the numbers in 'output_errors'.

There is now a command-line option --zero-indexed and the addition is done in the function output_errors.

@JonatanWaern
Copy link
Contributor

Needs rebase on main branch and changelog entry. Otherwise looks good.

@jvsqzj
Copy link
Contributor Author

jvsqzj commented Sep 25, 2025

Needs rebase on main branch and changelog entry. Otherwise looks good.

Done here. I believe this is ready for review and merge.

@jvsqzj jvsqzj force-pushed the fix-off-by-one-line-reporting branch from 4e6cbb4 to ee42ff9 Compare September 25, 2025 18:30
@JonatanWaern JonatanWaern merged commit 12d3c11 into intel:main Sep 29, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants