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.

Sign up for GitHub

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

Jump to bottom

capi: Cleanup in fizzy.h comments #681

Merged
merged 4 commits into from
Jan 19, 2021
Merged

capi: Cleanup in fizzy.h comments #681

merged 4 commits into from
Jan 19, 2021

Conversation

gumb0
Copy link
Collaborator

@gumb0 gumb0 commented Jan 6, 2021

No description provided.

@gumb0 gumb0 requested a review from axic January 6, 2021 18:06
@codecov
Copy link

codecov bot commented Jan 6, 2021 •
edited
Loading

Codecov Report

Merging #681 (70f31e0) into master (8922491) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #681   +/-   ##
=======================================
  Coverage   99.31%   99.31%           
=======================================
  Files          72       72           
  Lines       10257    10257           
=======================================
  Hits        10187    10187           
  Misses         70       70
Flag Coverage Δ
spectests 91.50% <ø> (ø)
unittests 99.31% <ø> (ø)

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

axic
axic reviewed Jan 6, 2021
View reviewed changes
include/fizzy/fizzy.h Outdated Show resolved Hide resolved
axic
axic reviewed Jan 6, 2021
View reviewed changes
include/fizzy/fizzy.h Outdated Show resolved Hide resolved
axic
axic reviewed Jan 6, 2021
View reviewed changes
include/fizzy/fizzy.h Outdated Show resolved Hide resolved
@gumb0 gumb0 force-pushed the capi-comments-cleanup branch from f6831cd to 1851730 Compare January 7, 2021 14:29
@gumb0 gumb0 requested review from chfast and axic January 7, 2021 14:30
chfast
chfast reviewed Jan 8, 2021
View reviewed changes
include/fizzy/fizzy.h Outdated
@@ -33,10 +33,10 @@ typedef struct FizzyExecutionResult
/// Whether execution ended with a trap.
bool trapped;
/// Whether function returned a value.
/// Equals false if trapped equals true.
/// Equals false if `trapped` equals true.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// Equals false if `trapped` equals true.
/// Equals false if #trapped equals true.

I'm not sure that the goal is here, but you can reference the class variable like this. See https://www.doxygen.nl/manual/autolink.html.

@gumb0 gumb0 force-pushed the capi-comments-cleanup branch from 1851730 to 11672a2 Compare January 11, 2021 15:45
@gumb0
Copy link
Collaborator Author

gumb0 commented Jan 11, 2021 •
edited
Loading

I changed backticks to proper links, as Pawel suggested, please take a look.

I'll squash two last commits.

chfast
chfast reviewed Jan 13, 2021
View reviewed changes
Copy link
Collaborator

@chfast chfast left a comment

Choose a reason for hiding this comment

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

I guess some doxygen CI build would be welcomed to see the changes.

include/fizzy/fizzy.h Outdated
@@ -19,7 +19,7 @@ typedef struct FizzyInstance FizzyInstance;

/// The data type representing numeric values.
///
/// i64 member is used to represent values of both i32 and i64 type.
/// The `i64` member is used to represent values of both i32 and i64 type.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// The `i64` member is used to represent values of both i32 and i64 type.
/// The #i64 member is used to represent values of both i32 and i64 type.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This didn't work when I tried it locally

Copy link
Collaborator

Choose a reason for hiding this comment

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

Works for me provided the i64 is itself documented.

include/fizzy/fizzy.h Outdated
/// @param module Pointer to module. If NULL is passed, function has no effect.
///
/// @note
/// Should be called unless @p module was passed to ::fizzy_instantiate.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// Should be called unless @p module was passed to ::fizzy_instantiate.
/// Should be called unless @p module was passed to fizzy_instantiate().

include/fizzy/fizzy.h Outdated
/// functions querying module info can still be called with @p module.
/// For simplicity a module cannot be shared among several instances (calling ::fizzy_instatiate
/// more than once with the same module results in undefined behaviour), but after
/// ::fizzy_instantiate functions querying module info can still be called with @p module.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// ::fizzy_instantiate functions querying module info can still be called with @p module.
/// fizzy_instantiate() functions querying module info can still be called with @p module.

axic
axic approved these changes Jan 14, 2021
View reviewed changes
Copy link
Member

@axic axic left a comment

Choose a reason for hiding this comment

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

Looks okay to me. The last two commits could be merged. Also is @p argname still prefererd or there's some other shorter notation based on this linking feature?

@gumb0 gumb0 force-pushed the capi-comments-cleanup branch from 11672a2 to a708591 Compare January 18, 2021 11:28
@gumb0 gumb0 changed the base branch from master to ci_doxygen January 18, 2021 11:31
@gumb0 gumb0 force-pushed the capi-comments-cleanup branch from a708591 to 5a1eed3 Compare January 18, 2021 18:36
@gumb0
Copy link
Collaborator Author

gumb0 commented Jan 18, 2021

Changed remaining backticks to links, changed ::function_name links to function_name(), squashed last two commits.

@gumb0
Copy link
Collaborator Author

gumb0 commented Jan 18, 2021

Looks okay to me. The last two commits could be merged. Also is @p argname still prefererd or there's some other shorter notation based on this linking feature?

Looks like @p is still the way to go, auto-linking feature doesn't support parameters.

@gumb0 gumb0 requested a review from chfast January 18, 2021 18:58
@gumb0 gumb0 changed the base branch from ci_doxygen to master January 19, 2021 10:58
@gumb0 gumb0 force-pushed the capi-comments-cleanup branch from 5a1eed3 to 078e7f0 Compare January 19, 2021 10:59
@gumb0 gumb0 force-pushed the capi-comments-cleanup branch from 078e7f0 to 70f31e0 Compare January 19, 2021 11:00
@gumb0 gumb0 merged commit 76a3b2a into master Jan 19, 2021
@gumb0 gumb0 deleted the capi-comments-cleanup branch January 19, 2021 12:00
@gumb0 gumb0 mentioned this pull request Jan 27, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@axic axic axic approved these changes

@chfast chfast chfast approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gumb0 @axic @chfast