Skip to content

document spork system with doxygen comments#2301

Merged
UdjinM6 merged 1 commit into
dashpay:developfrom
nmarley:spork-docs
Oct 2, 2018
Merged

document spork system with doxygen comments#2301
UdjinM6 merged 1 commit into
dashpay:developfrom
nmarley:spork-docs

Conversation

@nmarley
Copy link
Copy Markdown

@nmarley nmarley commented Sep 17, 2018

This is an initial shot at an on-going WIP, which will add documentation via Doxygen comments to (especially) Dash-specific features in the codebase.

@UdjinM6 I will need some guidance on the rationale for GetSignatureHash, as you will see. 😉

Copy link
Copy Markdown

@gladcow gladcow left a comment

Choose a reason for hiding this comment

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

May be it is better to place method descriptions in header file and to place only some implementation-specific notes in cpp file?

Comment thread src/spork.cpp Outdated
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

s/ensire/ensure/

Comment thread src/spork.cpp Outdated
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

"representations" should be singular

@nmarley
Copy link
Copy Markdown
Author

nmarley commented Sep 18, 2018

May be it is better to place method descriptions in header file and to place only some implementation-specific notes in cpp file?

Thanks for the suggestion. I added them with the implementations because I thought that made more sense, but maybe the actual C++ way is to add them in the header files. Since the classes were declared in header files, that's where I added those class comments. Technically the methods are "declared" in the class definition (e.g. the function prototypes).

I guess the big questions (for me) are:

  1. What is C++ convention for this?
  2. Where are the comments more helpful?

It's possible doxygen also cares where these go. Nope, looks like doxygen doesn't care. I see various opinions online. Wondering what others think.

@PastaPastaPasta
Copy link
Copy Markdown
Member

What is C++ convention for this?

Doesn't seem like there is one that I can find.

Found this that makes sense (just some guys opinion):

  • Document how to use the function in the header file, or more accurately close to the declaration.
  • Document how the function works (if it's not obvious from the code) in the source file, or more accurately, close to the definition.

@gladcow
Copy link
Copy Markdown

gladcow commented Sep 18, 2018

What is C++ convention for this?

There is no common convention for comments, but there is google styleguide, for example, https://google.github.io/styleguide/cppguide.html#Function_Comments or such guide: http://www.edparrish.net/common/cppdoc.html. The most common practice is describe function goal and usage conditions in header and non-obvious implementation details in cpp file, I think.

Where are the comments more helpful?

Function description in header is more helpful: when you see unknown function in code, you are clicking Go To Definition in your IDE and goes to header file usually.)

@nmarley
Copy link
Copy Markdown
Author

nmarley commented Sep 18, 2018

@gladcow The more I look over DashCore code, I'm realizing that I don't actually look in the header files. And in my IDE when I click to go to the function, it's taking me to the implementation (e.g. the cpp file), not the header declaration (I use CLion).

Personally I don't think it makes since to stick the documentation in the header files where I'm not even looking at the file. The comments make the most sense to me where the method is defined (not declared).

@nmarley
Copy link
Copy Markdown
Author

nmarley commented Sep 18, 2018

@PastaPastaPasta I'm almost certain your random guy's opinion came from StackExchange, which I also found yesterday as well, while searching conventions. I also found others which differ, also on Stack Exchange, such as this one (which I tend to agree with):

Put the documentation where people will read and write it as they are using and working on the code.

Class comments go in front of classes, method comments in front of methods.

That is the best way to make sure things are maintained. It also keeps your header files relatively lean and avoids the touching issue of people updating method docs causing headers to be dirty and triggering rebuilds. I have actually known people use that as an excuse for writing documentation later!

@nmarley nmarley changed the title [WIP] document spork system with doxygen comments document spork system with doxygen comments Sep 29, 2018
@nmarley
Copy link
Copy Markdown
Author

nmarley commented Sep 29, 2018

Rebased onto develop and moved method comments into header file per discussion. Also removed [WIP] tag, ready for review.

@UdjinM6 UdjinM6 added this to the 12.4 milestone Sep 29, 2018
Copy link
Copy Markdown

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

Looks good. Will require a bit more work after #2288. Added a couple of notes, see comments below.

Comment thread src/spork.h Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The signature field is not included in the serialization. this will not be true after #2288

Comment thread src/spork.h Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'm not sure why

The signature field is not included in the serialization. - that's why in this case ;)

Comment thread src/spork.h Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should probably clarify that this is only true for time-based sporks (which are majority), for pure value-based sporks like SPORK_5_INSTANTSEND_MAX_VALUE this function makes no sense and should not be used.

@nmarley
Copy link
Copy Markdown
Author

nmarley commented Sep 30, 2018

Fixed Get*Hash methods and added comment for CheckAndRemove, so that should be all (for now). I will start on the post-2288 work and rebase once that gets merged in.

This is an initial shot at an on-going WIP, which will add documentation via
Doxygen comments to (especially) Dash-specific features in the codebase.
@nmarley
Copy link
Copy Markdown
Author

nmarley commented Sep 30, 2018

Added comments for new methods introduced in #2288, squashed & rebased to resolve conflicts. Please re-review.

Copy link
Copy Markdown

@gladcow gladcow left a comment

Choose a reason for hiding this comment

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

ACK

Copy link
Copy Markdown

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

LGTM 👍

utACK

@UdjinM6 UdjinM6 merged commit 07208a4 into dashpay:develop Oct 2, 2018
CryptoCentric pushed a commit to absolute-community/absolute that referenced this pull request May 20, 2019
This is an initial shot at an on-going WIP, which will add documentation via
Doxygen comments to (especially) Dash-specific features in the codebase.
@nmarley nmarley deleted the spork-docs branch October 3, 2019 12:42
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.

4 participants