Skip to content

Conversation

@jmjatlanta
Copy link

Provides better documentation of checkpoints. No change in functionality

Copy link

@DeckerSU DeckerSU left a comment

Choose a reason for hiding this comment

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

Could you plz keep the original includes order and forward declarations in this PR. Leave only documentation additions. Also i would like to keep the following constructions:

#ifndef BITCOIN_CHECKPOINTS_H
#define BITCOIN_CHECKPOINTS_H
...
#endif // BITCOIN_CHECKPOINTS_H

Instead of just:

#pragma once

For the new source files i don't mind of #pragma once usage, but for sources inherited from Bitcoin, better to keep "existing order" of things. Just bcz it will help/promote to easy cherry-picking or merge some PRs from bitcoin upstream.

In other words i would like to keep document PRs - "document only". Such type of PRs should only add comments and don't touch includes, defines, pragmas, etc. IMHO.

@dimxy
Copy link
Collaborator

dimxy commented Jul 5, 2022

tbh i'd prefer to keep original files intact, if possible

@jmjatlanta jmjatlanta force-pushed the jmj_checkpoint_doc branch from f721ddd to d9a4421 Compare July 5, 2022 13:55
@jmjatlanta
Copy link
Author

I reverted the #pragma and removed the #include.

I understand an argument can be made to simply close this PR because it doesn't match the upstream Bitcoin repository's version. However, I believe there is value in documenting these things, and the forked code is so old that attempting to change upstream's version is futile and provides no benefit to the version we use.

However, I will happily accept any decision from the team.

@tonymorony
Copy link

these changes implemented in combined PR: #559

@tonymorony tonymorony closed this Sep 5, 2022
who-biz pushed a commit to who-biz/komodo that referenced this pull request Jun 12, 2023
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