Ticket #1137 (closed patch: fixed)

Opened 12 years ago

Last modified 12 years ago

[PATCH] fix codingstd/c_indent.t wrt switch/case labels

Reported by: plobsing Owned by: coke
Priority: minor Milestone:
Component: coding_standards Version: trunk
Severity: low Keywords:
Cc: coke Language:
Patch status: applied Platform:

Description

PDD07 Specifies:

C and Perl code must be indented four columns per nesting level.

and

Labels (including case labels) must be outdented two columns relative to the code they label.

However, the current coding standard tests check that all indented C lines be indented by a multiple of 4.

Indent by 4, outdent by 2, gives an indentation that is a multiple of 2 but not a multiple of four.

Attachments

prove.outdent.txt Download (3.9 KB) - added by jkeenan 12 years ago.
Result of applying this patch to trunk and running prove -v t/codingstd/c_indent.t
codingstd_outdent.patch Download (1.7 KB) - added by plobsing 12 years ago.
codingstd_outdent_v2.patch Download (1.7 KB) - added by plobsing 12 years ago.

Change History

Changed 12 years ago by jkeenan

Result of applying this patch to trunk and running prove -v t/codingstd/c_indent.t

  Changed 12 years ago by jkeenan

  • status changed from new to assigned

I tried this patch out in trunk and ran prove. The output is attached.

As it stands, I think the patch creates more problems than it solves. It flags as errors many code chunks that we probably don't think of as bad C code formatting. For example, it flags 3 errors in this code from src/io/sockets_unix.c:

171             case EINPROGRESS:
172                 goto AGAIN;     
173             case EISCONN:           
174                 return 0;
175             default:                
176                 return -1;
177         }                    

Here we have 3 lines ending in a colon outdented by 4 characters rather than 2. They pass the trunk version of c_indent.t but not the patched version. I suspect that the problem is that, intuitively, we only want LABELs to be outdented by 2; we want all other indentation to be by 4. I think this was what plobsing was trying to get at. However, it's very difficult to embody this precisely in code because it's difficult to say that one line ending in a colon is a LABEL whereas another, as in a switch statement, is not.

Other thoughts/opinions?

Thank you very much.

kid51

follow-up: ↓ 3   Changed 12 years ago by coke

On Fri, Oct 23, 2009 at 3:09 PM, Parrot <parrot-tickets@lists.parrot.org> wrote:
> #1137: [PATCH] fix codingstd/c_indent.t wrt switch/case labels
> ------------------------------+---------------------------------------------
>  Reporter:  plobsing          |       Owner:  jkeenan
>     Type:  patch             |      Status:  assigned
>  Priority:  minor             |   Milestone:
> Component:  coding_standards  |     Version:  1.6.0
>  Severity:  low               |    Keywords:
>     Lang:                    |       Patch:
>  Platform:                    |
> ------------------------------+---------------------------------------------
> Changes (by jkeenan):
>
>  * status:  new => assigned
>
>
> Comment:
>
>  I tried this patch out in trunk and ran `prove`.  The output is attached.
>
>  As it stands, I think the patch creates more problems than it solves.  It
>  flags as errors many code chunks that we probably don't think of as bad C
>  code formatting.  For example, it flags 3 errors in this code from
>  ''src/io/sockets_unix.c'':
>  {{{
>  171             case EINPROGRESS:
>  172                 goto AGAIN;
>  173             case EISCONN:
>  174                 return 0;
>  175             default:
>  176                 return -1;
>  177         }
>  }}}
>  Here we have 3 lines ending in a colon outdented by 4 characters rather
>  than 2.  They pass the trunk version of ''c_indent.t'' but not the patched
>  version.  I suspect that the problem is that, intuitively, we only want
>  '''LABELs''' to be outdented by 2; we want all other indentation to be by
>  4.  I think this was what plobsing was trying to get at.  However, it's
>  very difficult to embody this precisely in code because it's difficult to
>  say that one line ending in a colon is a LABEL whereas another, as in a
>  switch statement, is not.
>
>  Other thoughts/opinions?
>
>  Thank you very much.
>
>  kid51

Looks like making

if ($line =~ /:$/) {

slightly smarter would be sufficient; say, optional leading whitespace
followed by valid label characters ending with a colon - that should
be enough to stop it from catching 'case FOO:'.

-- 
Will "Coke" Coleda

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

Replying to coke:

Looks like making

if ($line =~ /:$/) {

slightly smarter would be sufficient; say, optional leading whitespace followed by valid label characters ending with a colon - that should be enough to stop it from catching 'case FOO:'.

This is by design. The spec says all labels and explicitly includes case labels:

Labels (including case labels) must be outdented two columns relative to the code they label.

  Changed 12 years ago by coke

  • cc coke added

in reply to: ↑ 3 ; follow-up: ↓ 6   Changed 12 years ago by coke

Replying to plobsing:

Replying to coke:

Looks like making

{{{ if ($line =~ /:$/) { }}}

slightly smarter would be sufficient; say, optional leading whitespace followed by valid label characters ending with a colon - that should be enough to stop it from catching 'case FOO:'.

This is by design. The spec says all labels and explicitly includes case labels:

Labels (including case labels) must be outdented two columns relative to the code they label.

Ah, so it does. Ok. Looking through all the reported failures, here's a case that appears to be valid but fails your check.

case FLOATTYPE_16: /* native */

If you can allow for the trailing comment, we apply your changes and reformat the labels/switch statements.

Thanks!

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

Replying to coke:

case FLOATTYPE_16: /* native */

If you can allow for the trailing comment, we apply your changes and reformat the labels/switch statements. Thanks!

Thanks for finding this edge case. Patch updated.

Changed 12 years ago by plobsing

Changed 12 years ago by plobsing

  Changed 12 years ago by jkeenan

I tried applying codingstd_outdent_v2.patch to trunk. These were my results:

t/codingstd/c_indent.t .. 
1..2
ok 1 - Correctly indented preprocessor directives
not ok 2 - Correctly indented C files

#   Failed test 'Correctly indented C files'
#   at t/codingstd/c_indent.t line 227.
# incorrect indenting in C file found 38 occurrences in 6 files:
# /home/jimk/work/parrot/src/packfile/pf_items.c:1543
#     apprent non-2 space outdenting (8 spaces)
#  /home/jimk/work/parrot/src/packfile/pf_items.c:1546
#     apprent non-2 space outdenting (8 spaces)
#  /home/jimk/work/parrot/src/packfile/pf_items.c:1554
#     apprent non-2 space outdenting (8 spaces)
#  /home/jimk/work/parrot/src/packfile/pf_items.c:1557
#     apprent non-2 space outdenting (8 spaces)
#  /home/jimk/work/parrot/src/packfile/pf_items.c:1581
#     apprent non-2 space outdenting (8 spaces)
#  /home/jimk/work/parrot/src/packfile/pf_items.c:1584
#     apprent non-2 space outdenting (8 spaces)
#  /home/jimk/work/parrot/src/packfile/pf_items.c:1592
#     apprent non-2 space outdenting (8 spaces)
#  /home/jimk/work/parrot/src/packfile/pf_items.c:1595
#     apprent non-2 space outdenting (8 spaces)
#  /home/jimk/work/parrot/src/packfile/pf_items.c:1623
#     apprent non-2 space outdenting (8 spaces)
#  /home/jimk/work/parrot/src/packfile/pf_items.c:1626
#     apprent non-2 space outdenting (8 spaces)
#  /home/jimk/work/parrot/src/packfile/pf_items.c:1634
#     apprent non-2 space outdenting (8 spaces)
#  /home/jimk/work/parrot/src/packfile/pf_items.c:1637
#     apprent non-2 space outdenting (8 spaces)
#  /home/jimk/work/parrot/src/packfile/pf_items.c:1645
#     apprent non-2 space outdenting (8 spaces)
#  /home/jimk/work/parrot/src/packfile/pf_items.c:1648
#     apprent non-2 space outdenting (8 spaces)
#  /home/jimk/work/parrot/src/packfile/pf_items.c:1672
#     apprent non-2 space outdenting (8 spaces)
#  /home/jimk/work/parrot/src/packfile/pf_items.c:1675
#     apprent non-2 space outdenting (8 spaces)
#  /home/jimk/work/parrot/src/packfile/pf_items.c:1683
#     apprent non-2 space outdenting (8 spaces)
#  /home/jimk/work/parrot/src/packfile/pf_items.c:1685
#     apprent non-2 space outdenting (8 spaces)
#  /home/jimk/work/parrot/src/packfile/pf_items.c:1693
#     apprent non-2 space outdenting (8 spaces)
#  /home/jimk/work/parrot/src/packfile/pf_items.c:1696
#     apprent non-2 space outdenting (8 spaces)
#  /home/jimk/work/parrot/src/string/charset/unicode.c:843
#     apprent non-2 space outdenting (12 spaces)
#  /home/jimk/work/parrot/src/events.c:944
#     apprent non-2 space outdenting (12 spaces)
#  /home/jimk/work/parrot/src/events.c:946
#     apprent non-2 space outdenting (12 spaces)
#  /home/jimk/work/parrot/src/events.c:963
#     apprent non-2 space outdenting (32 spaces)
#  /home/jimk/work/parrot/src/io/socket_unix.c:171
#     apprent non-2 space outdenting (12 spaces)
#  /home/jimk/work/parrot/src/io/socket_unix.c:173
#     apprent non-2 space outdenting (12 spaces)
#  /home/jimk/work/parrot/src/io/socket_unix.c:175
#     apprent non-2 space outdenting (12 spaces)
#  /home/jimk/work/parrot/src/io/socket_unix.c:328
#     apprent non-2 space outdenting (12 spaces)
#  /home/jimk/work/parrot/src/io/socket_unix.c:378
#     apprent non-2 space outdenting (12 spaces)
#  /home/jimk/work/parrot/src/io/socket_win32.c:118
#     apprent non-2 space outdenting (12 spaces)
#  /home/jimk/work/parrot/src/io/socket_win32.c:120
#     apprent non-2 space outdenting (12 spaces)
#  /home/jimk/work/parrot/src/io/socket_win32.c:122
#     apprent non-2 space outdenting (12 spaces)
#  /home/jimk/work/parrot/src/io/socket_win32.c:275
#     apprent non-2 space outdenting (12 spaces)
#  /home/jimk/work/parrot/src/io/socket_win32.c:325
#     apprent non-2 space outdenting (12 spaces)
#  /home/jimk/work/parrot/src/io/unix.c:517
#     apprent non-2 space outdenting (12 spaces)
#  /home/jimk/work/parrot/src/io/unix.c:554
#     apprent non-2 space outdenting (4 spaces)
#  /home/jimk/work/parrot/src/io/unix.c:607
#     apprent non-2 space outdenting (12 spaces)
#  /home/jimk/work/parrot/src/io/unix.c:617
#     apprent non-2 space outdenting (12 spaces)
# Looks like you failed 1 test of 2.
Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/2 subtests 

Test Summary Report
-------------------
t/codingstd/c_indent.t (Wstat: 256 Tests: 2 Failed: 1)
  Failed test:  2
  Non-zero exit status: 1
Files=1, Tests=2,  2 wallclock secs ( 0.02 usr  0.00 sys +  0.65 cusr  0.00 csys =  0.67 CPU)
Result: FAIL

Patch not applied.

  Changed 12 years ago by coke

  • owner changed from jkeenan to coke
  • status changed from assigned to new

  Changed 12 years ago by coke

  • version changed from 1.6.0 to trunk
  • patch set to applied

Patch applied (plus typo fix) in r42332;

I also modified the 'dent levels as specified (correctly) in the new test.

Thank you for the patch!

  Changed 12 years ago by coke

  • status changed from new to closed
  • resolution set to fixed
Note: See TracTickets for help on using tickets.