Add support for arbitrary expressions as default values in documentation #1323

malthe · 2016-10-15T09:11:24Z

This fixes an issue where a recover-expression would get printed
as simply "recover", leaving out both the actual expression as well
as the "end" token.

The doc_expression function writes out an expression provided
as an AST node to the provided document.

This closes #1262.

malthe · 2016-10-15T09:12:27Z

This is somewhat preliminary and incomplete, but it does cover the situation stated in the issue. I wanted to at least get some feedback if this is the right approach.

SeanTAllen · 2016-10-15T11:07:26Z

@malthe are you saying this shouldn't be merged?

SeanTAllen · 2016-10-15T11:10:56Z

@malthe are far more descriptive commit message would be desired: "closes X" isnt particularly helpful. The summation of the problem solved should be included in the commit comment. GH issues can go away, the commit comment will live on. Can you amend to include a full description of the problem and the approach taken to resolve?

SeanTAllen · 2016-10-15T11:11:56Z

src/libponyc/ast/ast.c

+  switch(token_get_id(ast->t))
+  {
+    case TK_STRING:
+    {


whats up with the {}'s around this case statement?

AFAIK you can't define a symbol in a case statement without wrapping in curly braces.

SeanTAllen · 2016-10-15T11:12:34Z

src/libponyc/ast/ast.c

+    {
+      char* escaped = token_print_escaped(ast->t);
+      size_t len = strlen(escaped);
+      char* s = (char*)ponyint_pool_alloc_size(len + 3);


its been a bit since i worked with this stuff, but i believe this needs to be freed somewhere.

SeanTAllen · 2016-10-15T11:13:13Z

src/libponyc/pass/docgen.c

@@ -548,6 +548,82 @@ static void doc_type_params(docgen_t* docgen, ast_t* t_params,
    fprintf(docgen->type_file, "]");
 }

+// Write the expression to the current type file.
+void doc_fprint(docgen_t* docgen, ast_t* ast)


why fprint as a name?

to me from doc_fprint, its certainly not clear that this is writing out an expression.

SeanTAllen · 2016-10-15T11:14:46Z

What does the output expression look like if this is used?

malthe · 2016-10-15T12:15:52Z

The output now looks like this:

new val create(
  base: (FilePath val | AmbientAuth val),
  path': String val,
  caps': Flags[(FileCreate val | FileChmod val | FileChown val | 
    FileLink val | FileLookup val | FileMkdir val | 
    FileRead val | FileRemove val | FileRename val | 
    FileSeek val | FileStat val | FileSync val | 
    FileTime val | FileTruncate val | FileWrite val | 
    FileExec val), U32 val] val = recover val FileCaps.all() end)
: FilePath val^ ?

The interesting bit here being the recover default value expression.

I think this pull request can be considered for merging into the main tree with the caveat that the implementation is not complete in the sense that not all expressions are correctly printed.

src/libponyc/pass/docgen.c

SeanTAllen · 2016-10-15T12:20:27Z

src/libponyc/pass/docgen.c

+      fprintf(fp, "recover");
+      while(child != NULL)
+        {
+          if (ast_id(child) != TK_NONE) {


standard pony c style formatting for this is something like

if(docgen.index_file != NULL && docgen.home_file != NULL) {

src/libponyc/pass/docgen.c

SeanTAllen · 2016-10-15T12:22:00Z

src/libponyc/ast/token.c

+{
+  char* escaped = token_print_escaped(token);
+  size_t len = strlen(escaped);
+  char* s = (char*)ponyint_pool_alloc_size(len + 3);


where does this get freed?

This gets freed in token_free because we assign it to token->printed.

Won't this leak the previous token->printed if one was assigned?

Perhaps it's better to use the same semantics as token_print_escaped where the caller must free it explicitly.

SeanTAllen · 2016-10-15T16:40:04Z

@malthe this is segfaulting now.

malthe · 2016-10-16T10:47:42Z

It seems like net/TCP.expect still fails every once in a while.

SeanTAllen · 2016-10-16T11:31:51Z

src/libponyc/ast/token.c

@@ -219,7 +219,6 @@ char* token_print_escaped(token_t* token)
  return escaped;
 }

-


why was this line removed? i assume it was a mistake and should be added back in.

SeanTAllen · 2016-10-16T11:33:07Z

src/libponyc/pass/docgen.c

+      {
+        doc_expression(docgen, child);
+        child = ast_sibling(child);
+        if (child != NULL) {


standard ponyc still is for npo {}'s when its a single line after the if

additionally, no space between the if and the (child != NULL) is the standard style so

if(child!=NULL)

SeanTAllen · 2016-10-16T11:34:01Z

src/libponyc/pass/docgen.c

+      {
+        doc_expression(docgen, child);
+        child = ast_sibling(child);
+        if (child != NULL) {


standard ponyc style is for no braces with a single line if

additionally, no space between the if and the (child != NULL) is the standard style so

if(child!=NULL)

SeanTAllen · 2016-10-16T11:36:24Z

src/libponyc/pass/docgen.c

+      doc_type(docgen, ast, false);
+      break;
+
+    case TK_DOT:


This could be combined with TK_SEQ and TK_CALL and others that are the same no? There's a slight variance in character that would have to be handled but there's a ton of duplication and it would be easy to introduce bugs in this.

SeanTAllen · 2016-10-16T11:39:04Z

src/libponyc/pass/docgen.c

+      fprintf(fp, "recover");
+      while(child != NULL)
+      {
+        if (ast_id(child) != TK_NONE) {


this seems like the wrong way to tackle this.

there could be other types later that we would be special casing for. that said, i dont immediately have an idea for a better way to address.

SeanTAllen · 2016-10-16T11:40:31Z

src/libponyc/pass/docgen.c

+      while(child != NULL)
+      {
+        doc_expression(docgen, child);
+        child = ast_sibling(child);


why does this not have the NULL check logic that others do?

This fixes an issue where a recover-expression would get printed as simply "recover", leaving out both the actual expression as well as the "end" token. The `doc_expression` function writes out an expression provided as an AST node to the provided document.

mfelsche · 2018-09-26T10:29:30Z

src/libponyc/pass/docgen.c

+        }
+      }
+      if (token == TK_CALL)
+        fprintf(fp, "()");


Where are call arguments handled?

mfelsche · 2018-09-26T10:32:14Z

Afaik this does not handle any control structures, like if or loops or try statements. Are these in the scope of this PR?

malthe · 2018-09-26T10:52:53Z

@mfelsche I am not sure what the scope should be – the primary concern is to correctly print the expressions that appear in the standard library code as "default expression". I don't think there are any calls with arguments or control structures in there.

SeanTAllen · 2020-04-07T03:34:13Z

Closing as stale.

SeanTAllen reviewed Oct 15, 2016

View reviewed changes

malthe force-pushed the issue-1262 branch from dfaadee to 7a60b60 Compare October 15, 2016 12:11

malthe changed the title ~~Support for writing out expressions in documentation~~ Add support for arbitrary expressions as default values in documentation Oct 15, 2016

SeanTAllen requested changes Oct 15, 2016

View reviewed changes

malthe force-pushed the issue-1262 branch 4 times, most recently from 8b39db2 to 94aaf6a Compare October 15, 2016 21:12

SeanTAllen reviewed Oct 16, 2016

View reviewed changes

SeanTAllen force-pushed the master branch 6 times, most recently from 97b1943 to 21d37aa Compare March 11, 2017 14:51

SeanTAllen added the stale label Apr 5, 2017

mfelsche mentioned this pull request Nov 29, 2017

Formatting of stdlib doc for Array.slice() is incorrect #2384

Closed

malthe force-pushed the issue-1262 branch from 94aaf6a to 96b7ef9 Compare September 25, 2018 11:03

malthe added 4 commits September 25, 2018 15:15

Reinsert newline

752ad2f

Fix assert statement

06fce0f

Refactor and add support for chaining

d1346ea

malthe force-pushed the issue-1262 branch from 96b7ef9 to d1346ea Compare September 25, 2018 13:15

malthe mentioned this pull request Sep 26, 2018

Invalid documentation generated #1262

Open

mfelsche removed the stale label Sep 26, 2018

mfelsche reviewed Sep 26, 2018

View reviewed changes

src/libponyc/pass/docgen.c

}

}

if (token == TK_CALL)

fprintf(fp, "()");

Copy link

Contributor

mfelsche Sep 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where are call arguments handled?

presidentbeef mentioned this pull request Oct 7, 2019

Generated documentation default value display bug #3323

Open

SeanTAllen added the stale label Apr 7, 2020

SeanTAllen closed this Apr 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for arbitrary expressions as default values in documentation #1323

Add support for arbitrary expressions as default values in documentation #1323

malthe commented Oct 15, 2016 •

edited

Loading

malthe commented Oct 15, 2016

SeanTAllen commented Oct 15, 2016

SeanTAllen commented Oct 15, 2016

SeanTAllen Oct 15, 2016

malthe Oct 15, 2016

SeanTAllen Oct 15, 2016

SeanTAllen Oct 15, 2016

SeanTAllen Oct 15, 2016

SeanTAllen commented Oct 15, 2016

malthe commented Oct 15, 2016

SeanTAllen Oct 15, 2016

SeanTAllen Oct 15, 2016

malthe Oct 15, 2016

jemc Oct 15, 2016

malthe Oct 15, 2016

SeanTAllen commented Oct 15, 2016

malthe commented Oct 16, 2016

SeanTAllen Oct 16, 2016 •

edited

Loading

SeanTAllen Oct 16, 2016 •

edited

Loading

SeanTAllen Oct 16, 2016

SeanTAllen Oct 16, 2016

SeanTAllen Oct 16, 2016

SeanTAllen Oct 16, 2016

mfelsche Sep 26, 2018

mfelsche commented Sep 26, 2018

malthe commented Sep 26, 2018

SeanTAllen commented Apr 7, 2020

		@@ -219,7 +219,6 @@ char* token_print_escaped(token_t* token)
		return escaped;
		}

Add support for arbitrary expressions as default values in documentation #1323

Add support for arbitrary expressions as default values in documentation #1323

Conversation

malthe commented Oct 15, 2016 • edited Loading

malthe commented Oct 15, 2016

SeanTAllen commented Oct 15, 2016

SeanTAllen commented Oct 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SeanTAllen commented Oct 15, 2016

malthe commented Oct 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SeanTAllen commented Oct 15, 2016

malthe commented Oct 16, 2016

SeanTAllen Oct 16, 2016 • edited Loading

Choose a reason for hiding this comment

SeanTAllen Oct 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfelsche commented Sep 26, 2018

malthe commented Sep 26, 2018

SeanTAllen commented Apr 7, 2020

malthe commented Oct 15, 2016 •

edited

Loading

SeanTAllen Oct 16, 2016 •

edited

Loading

SeanTAllen Oct 16, 2016 •

edited

Loading