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

Fix comment conversion in actor-cache.h. #1183

Merged
merged 1 commit into from
Sep 20, 2023

Conversation

kentonv
Copy link
Member

@kentonv kentonv commented Sep 14, 2023

This set of three methods had been grouped together, with a single comment under them applying to all three. d3f25fc broke apart the group and placed the comment so it appears to apply only to the third method.

I'm not sure if there may have been more instances like this around the codebase. I just happened to see this.

@ohodson
Copy link
Contributor

ohodson commented Sep 15, 2023

This set of three methods had been grouped together, with a single comment under them applying to all three. d3f25fc broke apart the group and placed the comment so it appears to apply only to the third method.

Perhaps we should go further here, as we are updating it, because clangd does not detect that the comment applies to the three methods here and part of the reasoning for going with a more traditional comment style was to integrate better with IDEs. I've added some tentative suggestions with that in mind.

I did not find which PITR API the original comment was referring to, but that's my bad.

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM with @ohodson's suggestions

@kentonv
Copy link
Member Author

kentonv commented Sep 18, 2023

I don't agree with the suggestions: It makes it harder for a human to read the header, adding noise.

The benefit for clangd is not worth it. The worst case is that a human gets no tooltip when hovering over a call and instead has to jump-to-definition. Unlike with the old comment style, clangd will not display the wrong comment as a result of this.

Being able to group declarations and comment on them as one is important to allow humans to quickly understand what they are looking at, without having to read each comment individually.

@jasnell
Copy link
Member

jasnell commented Sep 19, 2023

I'm not going to block on it. I prefer the suggestions but it's not worth blocking on so I'm good with this landing either way.

@ohodson
Copy link
Contributor

ohodson commented Sep 19, 2023

Okay here if you reject the comments.

The methods above this little block in actor-cache.h have helpful comments (much more so than my suggestions), and so do the comments in actor-state.h introduced in https://github.com/cloudflare/workerd/pull/744/files. It might be reasonable to have a pointer to those as an example implementation or a concrete pointer to the API. I don't know, it seems somewhat intuitive, just riffing on options.

@kentonv
Copy link
Member Author

kentonv commented Sep 20, 2023

I'm sure the comments could always be better but my intent with this PR was just to fix a regression in the comment understandability that was introduced when the comment style was changed. This bring the comment back to representing exactly the same information as it did before (except with the above-declaration style rather than below-declaration).

@kentonv kentonv merged commit 98689f5 into main Sep 20, 2023
7 checks passed
@kentonv kentonv deleted the kenton/fix-actorcache-comments branch September 20, 2023 22:35
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