-
Notifications
You must be signed in to change notification settings - Fork 141
javadoc for base [graph] #548
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
base: main
Are you sure you want to change the base?
Conversation
|
Before you submit for review:
If you did not complete any of these, then please explain below. |
marianotepper
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had a couple of comments. It looks good overall.
| * @param <T> the type of the output writer | ||
| */ | ||
| public abstract class AbstractGraphIndexWriter<T extends IndexWriter> implements GraphIndexWriter { | ||
| /** EOF magic number. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This magic number does not mark EOF.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the next line 33, incorrect then?
public static final int FOOTER_MAGIC = 0x4a564244; // "EOF magic"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would you suggest as a better description here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a magic marker that is used to check for the presence of a footer.
| * @param startOffset the start offset | ||
| * @throws IOException if an I/O error occurs | ||
| */ | ||
| public synchronized void writeHeader(ImmutableGraphIndex.View view, long startOffset) throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this methods be made protected? @sam-herman @michaeljmarshall do we really need it public?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marianotepper can we make the doc improvements anyway, given these two things are orthogonal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather not, because the javadoc is trying to explain why the method is public, which may be just a fluke.
This is the 2nd of the javadoc backfill PRs, covering only minor elisions in the base graph package.