Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New function intro comment format for Rebol C source #27

Closed
codebybrett opened this issue Oct 8, 2015 · 16 comments
Closed

New function intro comment format for Rebol C source #27

codebybrett opened this issue Oct 8, 2015 · 16 comments

Comments

@codebybrett
Copy link

A new commenting format is proposed for the Rebol C sources for which your acceptance is sought.

Example:

//
//  quit: native [
//  
//  {Stop evaluating and return control to command shell or calling script.}
//  
//      /with {Yield a result (mapped to an integer if given to shell)}
//      value [any-type!] "See: http://en.wikipedia.org/wiki/Exit_status"
//      /return "(deprecated synonym for /WITH)"
//      return-value
//  ]
// 
// While QUIT is implemented via a THROWN() value that bubbles up
// through the stack, it may not ultimately use the WORD! of QUIT
// as its /NAME when more specific values are allowed as names.
//
REBNATIVE(quit)
{

To see the effect of this in context with other functions see the draft conversions at: https://github.com/codebybrett/temporary.201508-source-format-change/tree/master/src

An introductory comment to a function declaration will contain rebol function metadata indented from slash by two spaces, a blank line, then any ordinary comments set off from double slash by a single space. Extra blank lines around the function description make easier reading.

Your opinions would be welcome in the chat room.

@hostilefork
Copy link

@zsx We are planning on going forward with this conversion pretty soon...right now I am trying to sort of tie up old unfinished branches just because it will be more of a pain to merge them later (every line being a conflict, we are doing the tab-to-spaces conversion here too...)

I don't know if that applies to anything you want to do before we do it. Let us know!

Git does have some abilities to ignore whitespace, I don't know much about how they work though.

@zsx
Copy link
Owner

zsx commented Nov 3, 2015

I would like it to be more clear about what's comments taken by rebol and what's not. One space is not that obvious. What happened to "//;"? I think I liked it better. But I am OK with this, if that's what you guys agreed upon.

Thanks for the reminder, BTW.

@codebybrett
Copy link
Author

The double newline after the rebol values separates the textual notes.

The parser test as currently written goes like this:

  1. Gather rebol values while you have not hit a double newline sequence. E.g the 3 values:

    quit: native [...]

  2. If the first rebol value gathered is a set-word! then we have validly formatted rebol metadata.

The spacing was thought to give a clean visual look, while minimising the length of lines, minimising special separators and highlighting function names.

Looking through the conversion I think this example may demonstrate your point, but I think in this case the notes should be manually reformatted to be clearer with the new format rather than change the format itself:

//
//  Check_Func_Spec: C
// 
// Check function spec of the form:
// 
// ["description" arg "notes" [type! type2! ...] /ref ...]
// 
// Throw an error for invalid values.
//
REBSER *Check_Func_Spec(REBSER *spec, REBYTE *exts)
{

@hostilefork
Copy link

I don't know how often putting comments in the header will be necessary, as I feel the native spec itself being in the comment...and generating the code...is about the best "comment" on a native definition we could ask for.

Most implementation comments are probably fine in the body. So maybe what we do is lean that direction, to make native comments just the native definition most of the time. That gives a good separation.

//
//  quit: native [
//  
//  {Stop evaluating and return control to command shell or calling script.}
//  
//      /with {Yield a result (mapped to an integer if given to shell)}
//      value [any-type!] "See: http://en.wikipedia.org/wiki/Exit_status"
//      /return "(deprecated synonym for /WITH)"
//      return-value
//  ]
// 
REBNATIVE(quit)
{
    // While QUIT is implemented via a THROWN() value that bubbles up
    // through the stack, it may not ultimately use the WORD! of QUIT
    // as its /NAME when more specific values are allowed as names.

How about that for the natives? I don't know what I think of it, but I like it better than (for instance) adding two more vertical lines of space and a --- divider, or something space-eating like that.

@zsx
Copy link
Owner

zsx commented Nov 10, 2015

I think I like this one. This makes the comment more for the implementation rather than the native interface.

@hostilefork
Copy link

@zsx Okay, well it does work for natives but with regular functions that will be maybe a bit strange. So like:

//
//  Push_Trap_Helper: c
//
void Push_Trap_Helper(REBOL_STATE *s) {
    // Used by both TRY and TRY_ANY, whose differentiation comes
    // from how they react to HALT.

Which isn't actually that bad either, I guess, if we start thinking that comment before is all metadata and is Rebol loadable. It's fairly stylized but stylized in a semi-meaningful way. Maybe I could get used to that.

The thing I am a broken record about is "modify with confidence" so it's not like just because we run the converter means we're stuck with it forever... Brett tackled this in a fairly generic way and if there turns out to be friction it can be modified.

@hostilefork
Copy link

@zsx @codebybrett I have essentially gotten everything ready to where I have no large pending commits to get out. If a call can be made on this issue of where to put the comments I'd be ready.

My suggestion is to split it down the middle and keep the current ability to use the format as designed, but in the case of natives in particular use a convention that the implementation documentation be in the body. So for the Push_Trap_Helper: c example, do not put the comments in the body.

Again--keep the format able to process it either way.

@codebybrett
Copy link
Author

On this basis, there's no need to change the existing parser for the build process. So I've modified the conversion script to generate this suggestion. Please see the commit for the updated draft . Perhaps n-control.c is a good example of this change.

@hostilefork
Copy link

I'm ready to sign off on Brett's compromise if you are, @zsx.

Also, your concerns won on the errors. I deleted my trick, used parentheses for the macros, and now they are very nice. :-)

@zsx
Copy link
Owner

zsx commented Nov 16, 2015

When I give it another look, the following looks better to me, and it's more aligned with the format in src/os/, e.g. https://github.com/metaeducation/ren-c/blob/master/src/os/posix/host-library.c

Putting the comment after "{" makes it more "local" (pertinent to the code following it) rather than "global" (pertinent to the whole function).

//
//  quit: native [
//  
//  {Stop evaluating and return control to command shell or calling script.}
//  
//      /with {Yield a result (mapped to an integer if given to shell)}
//      value [any-type!] "See: http://en.wikipedia.org/wiki/Exit_status"
//      /return "(deprecated synonym for /WITH)"
//      return-value
//  ]
// 
REBNATIVE(quit)
//  
//  While QUIT is implemented via a THROWN() value that bubbles up
//  through the stack, it may not ultimately use the WORD! of QUIT
//  as its /NAME when more specific values are allowed as names.
{

@hostilefork
Copy link

@zsx Hmmm.... I see your point. It's unusual but so is everything else we're doing.

Our tab point hack is lost here, though I do agree that it looks pretty good... but we're not going to start indenting all our // comments to a tab point. I was suggesting we only do tab points on code.

@hostilefork
Copy link

@zsx Or, to put that another way, would you accept:

//
//  quit: native [
//  
//  {Stop evaluating and return control to command shell or calling script.}
//  
//      /with {Yield a result (mapped to an integer if given to shell)}
//      value [any-type!] "See: http://en.wikipedia.org/wiki/Exit_status"
//      /return "(deprecated synonym for /WITH)"
//      return-value
//  ]
// 
REBNATIVE(quit)
//  
// While QUIT is implemented via a THROWN() value that bubbles up
// through the stack, it may not ultimately use the WORD! of QUIT
// as its /NAME when more specific values are allowed as names.
{

I guess I just want to avoid the slippery slope of indenting one's day to day comments on tab points, because...why? The tab point is because you want to align with another tab point in the ensuing information.

@zsx
Copy link
Owner

zsx commented Nov 17, 2015

Yes, I am OK with this one.

@hostilefork
Copy link

@codebybrett Okay, I can work with that if @zsx says yea. :-) Whenever you're ready.

Death to tabs!

codebybrett pushed a commit to codebybrett/rebol-source-scripting that referenced this issue Nov 17, 2015
@codebybrett
Copy link
Author

Conversion of comments and tabs applied in https://github.com/codebybrett/ren-c/tree/20151117-source-conversion

@hostilefork
Copy link

Now in master!

metaeducation@e6172c2

zsx added a commit that referenced this issue Nov 5, 2018
a simple "nl" in a text block could crash it, because it's a static
string, and not supposed to be free'd. "\n" is also supposed to be of
type wide_t*, or there'll be garbage text.

(gdb) bt
    #0  0x00007ffff72b2f73 in free () from /usr/lib/libc.so.6
    #1  0x00005555555fd956 in OS_Free (mem=0x5555556c49f8) at ../src/os/linux/host-lib.c:392
    #2  0x0000555555687413 in agg::rich_text::rt_reset (this=0x5555558f7d60) at ../src/agg/agg_truetype_text.cpp:135
    #3  0x0000555555652017 in agg::agg_graphics::agg_text (this=0x5555557ebde0, vectorial=1, p1=0x7fffffffc308, p2=0x7fffffffc310, block=0x7ffff61d6020) at ../src/agg/agg_graphics.cpp:1955
    #4  0x0000555555626103 in agg::rebdrw_text (gr=0x5555557ebde0, mode=1, p1=0x7fffffffc308, p2=0x7fffffffc310, block=0x7ffff61d6020) at ../src/os/host-draw-api-agg.cpp:413
    #5  0x0000555555623c01 in RXD_Draw (cmd=33, frm=0x7fffffffc3e0, ctx=0x7fffffffc5d0) at ../src/os/host-draw.c:571
    #6  0x000055555558b577 in Do_Commands (cmds=0x7ffff61d5fc0, context=0x7fffffffc5d0) at ../src/core/f-extension.c:585
    #7  0x000055555556b8a7 in RL_Do_Commands (blk=0x7ffff61d5fc0, flags=0, context=0x7fffffffc5d0) at ../src/core/a-lib.c:402
    #8  0x0000555555627b27 in agg::rebdrw_gob_draw (gob=0x555555940798, buf=0x7ffff5add000 '\377' <repeats 200 times>..., buf_size=..., abs_oft=..., clip_oft=..., clip_siz=...)
        at ../src/os/host-draw-api-agg.cpp:611
    #9  0x000055555562abf9 in process_gobs (ctx=0x5555559091f0, gob=0x555555940798) at ../src/os/linux/host-compositor.c:524
    #10 0x000055555562ae48 in process_gobs (ctx=0x5555559091f0, gob=0x555555940744) at ../src/os/linux/host-compositor.c:563
    #11 0x000055555562ae48 in process_gobs (ctx=0x5555559091f0, gob=0x5555559407ec) at ../src/os/linux/host-compositor.c:563
    #12 0x000055555562c037 in rebcmp_compose (ctx=0x5555559091f0, winGob=0x5555559407ec, gob=0x5555559407ec, only=0 '\000') at ../src/os/linux/host-compositor.c:691
    #13 0x000055555561affd in Draw_Window (wingob=0x5555559407ec, gob=0x5555559407ec) at ../src/os/host-view.c:225
    #14 0x000055555561b1a3 in Show_Gob (gob=0x5555559407ec) at ../src/os/host-view.c:288
    #15 0x000055555561b5ed in RXD_Graphics (cmd=5, frm=0x7fffffffd190, data=0x0) at ../src/os/host-view.c:338
    #16 0x000055555558b01a in Do_Command (value=0x7ffff6209110) at ../src/core/f-extension.c:456
    #17 0x0000555555570768 in Do_Next (block=0x7ffff61d5c00, index=9, op=0) at ../src/core/c-do.c:886
    #18 0x0000555555570e42 in Do_Blk (block=0x7ffff61d5c00, index=7) at ../src/core/c-do.c:1017
    #19 0x00005555555790f4 in Do_Function (func=0x7ffff6209070) at ../src/core/c-function.c:415
    #20 0x0000555555570768 in Do_Next (block=0x7ffff61d9a60, index=3, op=0) at ../src/core/c-do.c:886
    #21 0x0000555555570587 in Do_Next (block=0x7ffff61d9a60, index=0, op=0) at ../src/core/c-do.c:860
    #22 0x0000555555570e42 in Do_Blk (block=0x7ffff61d9a60, index=0) at ../src/core/c-do.c:1017
    #23 0x00005555555a1bc3 in N_while (ds=0x7ffff6209010) at ../src/core/n-loop.c:690
    #24 0x0000555555578bdd in Do_Native (func=0x7ffff6208fb0) at ../src/core/c-function.c:289
    #25 0x0000555555570768 in Do_Next (block=0x7ffff61d9aa0, index=5, op=0) at ../src/core/c-do.c:886
    #26 0x0000555555570e42 in Do_Blk (block=0x7ffff61d9aa0, index=2) at ../src/core/c-do.c:1017
    #27 0x00005555555790f4 in Do_Function (func=0x7ffff6208ed0) at ../src/core/c-function.c:415
    #28 0x0000555555572734 in Apply_Function (wblk=0x7ffff61d9aa0, widx=0, func=0x7ffff6208ed0, args=0x7fffffffd530) at ../src/core/c-do.c:1528
    #29 0x0000555555572883 in Apply_Func (where=0x7ffff61d9aa0, func=0x555555844da0) at ../src/core/c-do.c:1555
    #30 0x000055555559e5d2 in N_wake_up (ds=0x7ffff6208db0) at ../src/core/n-io.c:415
    #31 0x0000555555578bdd in Do_Native (func=0x7ffff6208e10) at ../src/core/c-function.c:289
    #32 0x0000555555570768 in Do_Next (block=0x7ffff7fbf1e0, index=6, op=0) at ../src/core/c-do.c:886
    #33 0x000055555556fbe8 in Do_Args (func_offset=105, path=0x0, block=0x7ffff7fbf1e0, index=3) at ../src/core/c-do.c:668
    #34 0x00005555555706ba in Do_Next (block=0x7ffff7fbf1e0, index=2, op=0) at ../src/core/c-do.c:879
    #35 0x0000555555570e42 in Do_Blk (block=0x7ffff7fbf1e0, index=2) at ../src/core/c-do.c:1017
    #36 0x000055555559b221 in N_either (ds=0x7ffff6208bd0) at ../src/core/n-control.c:596
    #37 0x0000555555578bdd in Do_Native (func=0x7ffff6208c30) at ../src/core/c-function.c:289
    #38 0x0000555555570768 in Do_Next (block=0x7ffff7fbf220, index=15, op=0) at ../src/core/c-do.c:886
    #39 0x0000555555570e42 in Do_Blk (block=0x7ffff7fbf220, index=10) at ../src/core/c-do.c:1017
    #40 0x00005555555a1bc3 in N_while (ds=0x7ffff6208bd0) at ../src/core/n-loop.c:690
    #41 0x0000555555578bdd in Do_Native (func=0x7ffff6208b70) at ../src/core/c-function.c:289
    #42 0x0000555555570768 in Do_Next (block=0x7ffff7fbf2e0, index=12, op=0) at ../src/core/c-do.c:886
    #43 0x0000555555570e42 in Do_Blk (block=0x7ffff7fbf2e0, index=9) at ../src/core/c-do.c:1017
    #44 0x00005555555790f4 in Do_Function (func=0x7ffff62089d0) at ../src/core/c-function.c:415
    #45 0x0000555555572734 in Apply_Function (wblk=0x7ffff7fbf2e0, widx=0, func=0x7ffff62089d0, args=0x7fffffffda30) at ../src/core/c-do.c:1528
    #46 0x0000555555572883 in Apply_Func (where=0x7ffff7fbf2e0, func=0x555555844140) at ../src/core/c-do.c:1555
    #47 0x000055555557979c in Awake_System (ports=0x7ffff61d5640, only=0) at ../src/core/c-port.c:183
    #48 0x0000555555579890 in Wait_Ports (ports=0x7ffff61d5640, timeout=4294967295, only=0) at ../src/core/c-port.c:217
    #49 0x000055555559e463 in N_wait (ds=0x7ffff6208890) at ../src/core/n-io.c:374
    #50 0x0000555555578bdd in Do_Native (func=0x7ffff62088f0) at ../src/core/c-function.c:289
    #51 0x0000555555570768 in Do_Next (block=0x7ffff61d9800, index=2, op=0) at ../src/core/c-do.c:886
    #52 0x0000555555570e42 in Do_Blk (block=0x7ffff61d9800, index=0) at ../src/core/c-do.c:1017
    rebol#53 0x00005555555790f4 in Do_Function (func=0x7ffff6208870) at ../src/core/c-function.c:415
    rebol#54 0x0000555555570768 in Do_Next (block=0x7ffff61d92e0, index=1, op=0) at ../src/core/c-do.c:886
    rebol#55 0x0000555555570e42 in Do_Blk (block=0x7ffff61d92e0, index=0) at ../src/core/c-do.c:1017
    rebol#56 0x000055555559b300 in N_if (ds=0x7ffff6208730) at ../src/core/n-control.c:623
    rebol#57 0x0000555555578bdd in Do_Native (func=0x7ffff6208790) at ../src/core/c-function.c:289
    rebol#58 0x0000555555570768 in Do_Next (block=0x7ffff61d9300, index=50, op=0) at ../src/core/c-do.c:886
    rebol#59 0x0000555555570e42 in Do_Blk (block=0x7ffff61d9300, index=46) at ../src/core/c-do.c:1017
    rebol#60 0x00005555555790f4 in Do_Function (func=0x7ffff62085f0) at ../src/core/c-function.c:415
    rebol#61 0x0000555555570768 in Do_Next (block=0x7ffff61d60e0, index=28, op=0) at ../src/core/c-do.c:886
    rebol#62 0x0000555555570e42 in Do_Blk (block=0x7ffff61d60e0, index=26) at ../src/core/c-do.c:1017
    rebol#63 0x000055555559b02d in N_do (ds=0x7ffff6208470) at ../src/core/n-control.c:522
    rebol#64 0x0000555555578bdd in Do_Native (func=0x7ffff62084d0) at ../src/core/c-function.c:289
    rebol#65 0x0000555555570768 in Do_Next (block=0x7ffff7fc5140, index=6, op=0) at ../src/core/c-do.c:886
    rebol#66 0x0000555555570e42 in Do_Blk (block=0x7ffff7fc5140, index=3) at ../src/core/c-do.c:1017
    rebol#67 0x000055555559b221 in N_either (ds=0x7ffff6208370) at ../src/core/n-control.c:596
    rebol#68 0x0000555555578bdd in Do_Native (func=0x7ffff62083d0) at ../src/core/c-function.c:289
    rebol#69 0x0000555555570768 in Do_Next (block=0x7ffff7fc51c0, index=20, op=0) at ../src/core/c-do.c:886
    rebol#70 0x0000555555570e42 in Do_Blk (block=0x7ffff7fc51c0, index=11) at ../src/core/c-do.c:1017
    rebol#71 0x000055555559b221 in N_either (ds=0x7ffff6208270) at ../src/core/n-control.c:596
    rebol#72 0x0000555555578bdd in Do_Native (func=0x7ffff62082d0) at ../src/core/c-function.c:289
    rebol#73 0x0000555555570768 in Do_Next (block=0x7ffff7fc5200, index=11, op=0) at ../src/core/c-do.c:886
    rebol#74 0x0000555555570e42 in Do_Blk (block=0x7ffff7fc5200, index=5) at ../src/core/c-do.c:1017
    rebol#75 0x000055555559b221 in N_either (ds=0x7ffff6208170) at ../src/core/n-control.c:596
    rebol#76 0x0000555555578bdd in Do_Native (func=0x7ffff62081d0) at ../src/core/c-function.c:289
    rebol#77 0x0000555555570768 in Do_Next (block=0x7ffff7fc5240, index=76, op=0) at ../src/core/c-do.c:886
    rebol#78 0x0000555555570e42 in Do_Blk (block=0x7ffff7fc5240, index=71) at ../src/core/c-do.c:1017
    rebol#79 0x00005555555790f4 in Do_Function (func=0x7ffff6208090) at ../src/core/c-function.c:415
    rebol#80 0x0000555555572734 in Apply_Function (wblk=0x7ffff7fc5240, widx=0, func=0x7ffff6208090, args=0x7fffffffe350) at ../src/core/c-do.c:1528
    rebol#81 0x0000555555572a40 in Do_Sys_Func (inum=34) at ../src/core/c-do.c:1588
    rebol#82 0x0000555555573f89 in Init_Mezz (reserved=0) at ../src/core/c-do.c:2320
    rebol#83 0x000055555556b558 in RL_Start (
        bin=0x5555556b63a0 <Reb_Init_Code> "x\234\325=is\334\066\226\373\231\277\002\241kK\322x\350>t\330鉣rdg\354\335\\e{j>t\261RT\223\222\070\352&{H\266e\215\343\377\276\357\302ţ[\262\235\311l\245b\221 \360\360\360\200w\342\001\375\027uV\256o\253\374\362\252Q\257_|\367\363\017\352m\266\270*\312ey\231g\265:\216~*\337E\323\361\344\311hr8;\236\314\306_Gdz\361\070\310W\353\262jԪL7\313L̓&o\226\331L\205\f\343P\275\375\341ͻɣ\261ZWeS.ʥ\252\027W\331*\v\203wYU\347e1S\343G'\217&A\221\254\240Y\263\254\203\030\240\244\331\371\346r\026\024e\221", <incomplete sequence \331>..., len=9009, script=0x0, script_len=0, flags=0)
        at ../src/core/a-lib.c:193
    rebol#84 0x00005555555fb344 in main (argc=2, argv=0x7fffffffe708) at ../src/os/host-main.c:235
    (gdb) frame 2
    #2  0x0000555555687413 in agg::rich_text::rt_reset (this=0x5555558f7d60) at ../src/agg/agg_truetype_text.cpp:135
    135					OS_Free(attr.text);
    (gdb) p attr
    $1 = {index = 10, name = 0x5555557a8af0 "/usr/share/fonts/TTF/arial.ttf", name_free = false, bold = 0, italic = 0, underline = 1, size = 18, color = {r = 128 '\200', g = 128 '\200', b = 128 '\200',
        a = 255 '\377'}, offset_x = 2, offset_y = 2, space_x = 0, space_y = 0, shadow_x = 0, shadow_y = 0, shadow_color = {r = 0 '\000', g = 0 '\000', b = 0 '\000', a = 255 '\377'}, shadow_blur = 0,
      text = 0x5555556c49f8 L"\n\x4f000000\xcf000000", text_free = true, isPara = true, para = {origin_x = 2, origin_y = 2, margin_x = 2, margin_y = 2, indent_x = 0, indent_y = 0, tabs = 40, wrap = 1,
        scroll_x = 0, scroll_y = 0, align = 19, valign = 23}, asc = 14, desc = 2, char_height = 13}
    (gdb) quit
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

No branches or pull requests

3 participants