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

capi: Documentation cleanup #664

Merged
merged 3 commits into from
Dec 21, 2020
Merged

capi: Documentation cleanup #664

merged 3 commits into from
Dec 21, 2020

Conversation

gumb0
Copy link
Collaborator

@gumb0 gumb0 commented Nov 24, 2020

No description provided.

@gumb0
Copy link
Collaborator Author

gumb0 commented Nov 24, 2020

Question: do we need to mention that it supports only ASCII-subset of UTF-8 for names, or is it obvious from the fact that const char* represents strings?

@gumb0 gumb0 requested review from chfast and axic November 24, 2020 15:48
@codecov
Copy link

codecov bot commented Nov 24, 2020

Codecov Report

Merging #664 (1b81853) into master (433b846) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #664   +/-   ##
=======================================
  Coverage   99.27%   99.27%           
=======================================
  Files          71       71           
  Lines        9853     9853           
=======================================
  Hits         9782     9782           
  Misses         71       71           
Flag Coverage Δ
spectests 91.46% <ø> (ø)
unittests 99.27% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

/// imported_globals_size equals 0.
/// @param imported_globals_size Size of the imported global array. Can be zero.
/// @returns non-NULL pointer to instance in case of success, NULL otherwise.
/// @param module Pointer to module. Cannot be NULL.
Copy link
Member

Choose a reason for hiding this comment

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

Are we also keeping some specific width between @param and the name? We could say that and do it in the other PR too (#528).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I keep one space because this looks like a "command" @param name and this is always aligned.

@@ -32,10 +32,10 @@ typedef struct FizzyExecutionResult
{
/// Whether execution ended with a trap.
bool trapped;
/// Whether function returned a value. Valid only if trapped equals false.
/// Whether function returned a value.
bool has_value;
Copy link
Member

@axic axic Nov 24, 2020

Choose a reason for hiding this comment

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

So if we make this ambiguous here, why don't we state that if trapped = false, the other fields are undefined behaviour.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see this ambiguous. This is false if trapped == true. You can add this in the description.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added a note.

/// defined in module. Behaviour is undefined, if index is not valid according to
/// module definition.
/// @param module Pointer to module. Cannot be NULL.
/// @param func_idx Function index. Can be either index of an imported function or of a function
Copy link
Collaborator

Choose a reason for hiding this comment

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

IMHO it is fine to just keep <=2 spaces between param name and description.

Copy link
Member

Choose a reason for hiding this comment

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

I agree, vote for 2 spaces.

Also #664 (comment) ^^

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed both widths.

Copy link
Member

Choose a reason for hiding this comment

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

Oh sorry, I meant the 2 spaces for between @param and module, and 4 spaces between module and Pointer to module 😬

Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the point of additional fucking spaces?

Copy link
Member

@axic axic Nov 24, 2020

Choose a reason for hiding this comment

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

It was the style @gumb0 used half time and I found it rather readable. I don't care whatever is the decision as long as it is consistent.

Copy link
Collaborator Author

@gumb0 gumb0 Nov 25, 2020

Choose a reason for hiding this comment

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

I think it looks more readable when it's a bit more spaced out.

(what I originally used was starting comments on tab positions)

@gumb0 gumb0 force-pushed the capi-comment-cleanup branch 3 times, most recently from 6e5e2ae to c1f7355 Compare November 24, 2020 20:57
@gumb0 gumb0 requested review from axic and chfast November 24, 2020 21:14
@gumb0 gumb0 merged commit edd145b into master Dec 21, 2020
@gumb0 gumb0 deleted the capi-comment-cleanup branch December 21, 2020 17:53
@gumb0 gumb0 mentioned this pull request Dec 21, 2020
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.

3 participants