Ticket #1329 (closed patch: wontfix)

Opened 5 years ago

Last modified 5 years ago

[PATCH] extend scope of t/codingstd/c_indent.t

Reported by: plobsing Owned by: plobsing
Priority: normal Milestone:
Component: coding_standards Version: 1.8.0
Severity: medium Keywords:
Cc: Language:
Patch status: Platform:

Description

t/codingstd/c_indent.t currently only scans C code when the C preprocessor stack isn't empty (ie between #if and #endif).

Much of parrot's C code doesn't fall into that category and is currently unchecked and sometimes incorrectly indented (eg: src/ops/sys.ops, which uses a lot of 2-space indenting).

The attached patch tweaks t/codingstd/c_indent.t so that it no longer ignores such code.

Attachments

diff Download (0.6 KB) - added by plobsing 5 years ago.
c_indent.t.patch
c_indent.plobsing.results.txt.gz Download (8.5 KB) - added by jkeenan 5 years ago.
Output of c_indent.t after applying plobsing's diff
diff.2 Download (1.7 KB) - added by plobsing 5 years ago.
more permissive c_indent.t patch

Change History

Changed 5 years ago by plobsing

  • attachment diff Download added

c_indent.t.patch

in reply to: ↑ description ; follow-up: ↓ 3   Changed 5 years ago by jkeenan

Replying to plobsing:

The attached patch tweaks t/codingstd/c_indent.t so that it no longer ignores such code.

I applied your patch and then ran prove -v t/codingstd/c_indent.t. It now reports thousands of violation of the c_indent coding standard. (See attachment.) Is that what you expected?

Thank you very much.

kid51

Changed 5 years ago by jkeenan

Output of c_indent.t after applying plobsing's diff

  Changed 5 years ago by jkeenan

  • owner jkeenan deleted

in reply to: ↑ 1   Changed 5 years ago by plobsing

Replying to jkeenan:

I applied your patch and then ran prove -v t/codingstd/c_indent.t. It now reports thousands of violation of the c_indent coding standard. (See attachment.) Is that what you expected?

Yes. And looking at a (relatively small) sampling, I have yet to see any false-positives.

I can make a patch (or set of patches) to fix these. But the number of affected lines makes me think it might be hard to review.

I am open to suggestions.

follow-up: ↓ 5   Changed 5 years ago by coke

Is 0 an invalid indent level?

Also, this code:

    for (edge = unit->bb_list[loop_info->header]->pred_list;
         edge;
         edge = edge->pred_next) {

is lined up vertically to be slightly more aesthetically pleasing. A strict reading of the standard probably disallows this, but I think the alternative (in the general case) is probably a little less readable.

(This is not a vote, just an observation.)

in reply to: ↑ 4 ; follow-up: ↓ 6   Changed 5 years ago by plobsing

Replying to coke:

Is 0 an invalid indent level?

The heuristic that's being triggered for the code with 0-space indentation is roughly "the first line of a top level block must have exactly 4-spaces of indentation".

Most of these can be made to go away if we modify this to be the first non-comment line another dubious case is eliminated by ignoring yacc/lex %{ ... %} blocks as top level blocks.

Changed 5 years ago by plobsing

more permissive c_indent.t patch

in reply to: ↑ 5 ; follow-up: ↓ 7   Changed 5 years ago by jkeenan

Replying to plobsing: The first patch gave these aggregated results:

#   Failed test 'Correctly indented C files'
#   at t/codingstd/c_indent.t line 227.
# incorrect indenting in C file found 1573 occurrences in 111 files:

The second patch gave these:

#   Failed test 'Correctly indented C files'
#   at t/codingstd/c_indent.t line 229.
# incorrect indenting in C file found 1548 occurrences in 92 files:

There is a very practical problem here: If we were to accept this patch, who would we get to correct these 1548 errors?

(I am not going to do that :-).)

kid51

in reply to: ↑ 6   Changed 5 years ago by plobsing

Replying to jkeenan:

There is a very practical problem here: If we were to accept this patch, who would we get to correct these 1548 errors? (I am not going to do that :-).) kid51

I understand that it isn't really reasonable to expect someone else to fix this. I will (try to) fix the errors if/when I get a commit bit.

  Changed 5 years ago by coke

  • owner set to coke

  Changed 5 years ago by coke

  • owner changed from coke to plobsing

  Changed 5 years ago by plobsing

  • status changed from new to closed
  • resolution set to wontfix

I've tried to get this to work in a sane fashion. I have failed. Closing ticket.

Note: See TracTickets for help on using tickets.