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

cpu/apic: multiple trivial cleanups #412

Merged
merged 2 commits into from
Oct 23, 2024
Merged

Conversation

00xc
Copy link
Member

@00xc 00xc commented Jul 17, 2024

  • Add documentation for several functions.
  • Remove the pub qualifier from functions that are not used outside the APIC module.
  • Clean up formatting in some comments
  • Make a few variables immutable.

@msft-jlange
Copy link
Contributor

I'm not sure we want to spend much effort converting the number of spaces after periods in comments. The VIM editor, for example (which I use) defaults to two spaces after a period when joining two comment lines together. I don't object to changes to these comments but we could easily wind up wasting a lot of time with comment editing if we try to get strict about requiring specific rules.

@00xc
Copy link
Member Author

00xc commented Jul 17, 2024

I'm not sure we want to spend much effort converting the number of spaces after periods in comments. The VIM editor, for example (which I use) defaults to two spaces after a period when joining two comment lines together. I don't object to changes to these comments but we could easily wind up wasting a lot of time with comment editing if we try to get strict about requiring specific rules.

I just did a quick search & replace, I don't think we should enforce it as a rule.

@00xc 00xc added the needs-rebase The PR needs to be rebased to the latest upstream branch label Aug 29, 2024
@msft-jlange
Copy link
Contributor

@00xc, this has been pending update for a while. Would you like me to take over the PR and complete it, or did you want to continue to update it yourself?

Add documentation for functions that simply return a boolean value to
improve clarity. While we are at it, remove the `pub` qualifier from
functions that are not used outside the APIC module. Clean up as well
the formatting in some comments.

Signed-off-by: Carlos López <carlos.lopezr4096@gmail.com>
Use constructor methods and make two mutable variables immutable.

Signed-off-by: Carlos López <carlos.lopezr4096@gmail.com>
@00xc 00xc removed the needs-rebase The PR needs to be rebased to the latest upstream branch label Oct 11, 2024
@00xc
Copy link
Member Author

00xc commented Oct 11, 2024

@00xc, this has been pending update for a while. Would you like me to take over the PR and complete it, or did you want to continue to update it yourself?

Should be good to go now

@msft-jlange
Copy link
Contributor

I still don't like all of the double- to single-space changes because it introduces a lot of noise into the diff (it makes it harder to see the substantive changes because they're mixed in with a lot of whitespace changes) but the rest of the change looks good.

@00xc 00xc added the ready-to-merge PR is ready for merging into main branch label Oct 21, 2024
@joergroedel joergroedel merged commit 57eb32f into coconut-svsm:main Oct 23, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge PR is ready for merging into main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants