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.

Sign up for GitHub

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

Jump to bottom

Small improvements to internal documentation #120388

Closed

FractalFir wants to merge 1 commit into rust-lang:master from FractalFir:master

Contributor

FractalFir commented Jan 26, 2024

Hi!

While working on my project (rustc_codegen_clr) I have noticed the internal rustc documentation is lacking in some places. So, I have decided to add documentation for a few types/methods, and extend some of it by adding additional information.

Those commits add documentation to Instance, some methods of Ty, and extend documentation of FnSig and FnAbi to add info about some edge cases.

rustbot assigned estebank

Collaborator

rustbot commented Jan 26, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @estebank (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

rustbot added S-waiting-on-review T-compiler has-merge-commits S-waiting-on-author labels

Collaborator

rustbot commented Jan 26, 2024

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git rebase -i master
$ # delete any merge commits in the editor that appears
$ git push --force-with-lease

The following commits are merge commits:

6518b42

This comment has been minimized.

Sign in to view

FractalFir force-pushed the master branch from 6518b42 to 8aaf7a5 Compare

January 26, 2024 17:36

This comment has been minimized.

Sign in to view

Urgau reviewed

View reviewed changes

compiler/rustc_middle/src/ty/instance.rs Outdated Show resolved Hide resolved

compiler/rustc_middle/src/ty/instance.rs Outdated Show resolved Hide resolved

Contributor Author

FractalFir commented Jan 26, 2024

@rustbot review

This comment has been minimized.

Sign in to view

estebank reviewed

View reviewed changes

Contributor

estebank left a comment

General comment: I notice that a lot of the new docstrings are longer than 100 chars long. Could you break them up to take more than one line when appropriate?

Everywhere you have "/// Checks if" change for "/// true if".

Instead of "creates", use "create".

I noticed quite a few typos. I'm not one to complain about this in general, but please in the future take some care to do a cursory review of your own changes so that we can avoid the reviewers getting distracted by pointing them out instead of looking at the meat of the contribution.

compiler/rustc_target/src/abi/call/mod.rs Show resolved Hide resolved

compiler/rustc_middle/src/ty/util.rs

Comment on lines +1022 to +1155

		/// Returns the size and sign of an inieger type.
		/// If the type is not an intieger, will panic with message "non integer discriminant".

Contributor

estebank Jan 26, 2024

Suggested change

      
                /// Returns the size and sign of an inieger type.
          
                /// If the type is not an intieger, will panic with message "non integer discriminant".
          
                /// Returns the size and sign of an integer type or panic.

compiler/rustc_middle/src/ty/util.rs Show resolved Hide resolved

compiler/rustc_middle/src/ty/sty.rs Show resolved Hide resolved

compiler/rustc_middle/src/ty/sty.rs Show resolved Hide resolved

compiler/rustc_middle/src/ty/sty.rs

@@ @@ -2312,6 +2341,7 @@ impl<'tcx> Ty<'tcx> { @@
                       *self.kind() == Str
                   }
+                  /// Checks if a type is a parameter type, with index `index`.

Contributor

estebank Jan 26, 2024

This could do with a better explanation. What is index? An index into what?

compiler/rustc_middle/src/ty/sty.rs

                   #[inline]
                   pub fn is_phantom_data(self) -> bool {
                       if let Adt(def, _) = self.kind() { def.is_phantom_data() } else { false }
                   }
+                  /// Checks if this type is a boolean.

Contributor

estebank Jan 26, 2024

I appreciate you taking the time to document all these methods, but some like this one are somewhat self-explanatory and don't know how much value there is in documenting them. When you were working and navigating this codebase, was the lack of documentation on the is_* and expect_ methods a hindrance?

Contributor Author

FractalFir Jan 27, 2024

No, I have just added them since I was already documenting that file anyway.

I also thought that, in the future, #[warn(missing_docs)] or even #[deny(missing_docs)]could be added, ensuring documentation is always present. In such case, even small function would need documentation.

compiler/rustc_middle/src/ty/sty.rs Show resolved Hide resolved

compiler/rustc_middle/src/ty/sty.rs Show resolved Hide resolved

compiler/rustc_middle/src/ty/sty.rs Show resolved Hide resolved

estebank reviewed

View reviewed changes

compiler/rustc_middle/src/ty/sty.rs Show resolved Hide resolved

estebank reviewed

View reviewed changes

compiler/rustc_middle/src/ty/sty.rs

Comment on lines +1331 to +1279

+              /// NOTE: this signature may not match exactly with the signature of a compiled function.
+              /// Attributes like `#[track_caller]` may introduce additional arguments, which are not present here, but are represented in [`rustc_target::abi::call::FnAbi`].
+              /// If you are interested in the exact signature and the way arguments are aligned/passed, use [`rustc_target::abi::call::FnAbi`].

Contributor

estebank Jan 27, 2024

These are the kind of documentation additions that are very useful.

estebank reviewed

View reviewed changes

compiler/rustc_middle/src/ty/sty.rs Show resolved Hide resolved

estebank reviewed

View reviewed changes

compiler/rustc_middle/src/ty/instance.rs Show resolved Hide resolved

compiler/rustc_middle/src/ty/instance.rs Show resolved Hide resolved

FractalFir commented

View reviewed changes

Contributor Author

FractalFir left a comment •

edited

Loading

I agree with most of the comments. I believe mentioning the unusual panic message in int_size_and_signed is still worth keeping.
But if you still believe this info should not be there - I will remove it.

As for the typos - sorry, I will try to search for them better in the future. I have dyslexia and I just can't see some of those things. I am currently trying to get some spellcheck VS Code plugins to work, so this should not be a problem in the future.

Contributor Author

FractalFir commented Jan 28, 2024

@rustbot review

Contributor

bors commented Feb 8, 2024

☔ The latest upstream changes (presumably #120807) made this pull request unmergeable. Please resolve the merge conflicts.

Collaborator

rustbot commented Feb 9, 2024

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git rebase -i master
$ # delete any merge commits in the editor that appears
$ git push --force-with-lease

The following commits are merge commits:

b18c828

This comment has been minimized.

Sign in to view

FractalFir force-pushed the master branch from b18c828 to b61c1d6 Compare

February 9, 2024 19:52

Dylan-DPC removed the S-waiting-on-author label

Contributor

apiraino commented Apr 11, 2024

@FractalFir can you get the conflicts resolved when you have a chance? thanks

rustbot added the S-waiting-on-author label

Collaborator

rustbot commented Apr 11, 2024

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git rebase -i master
$ # delete any merge commits in the editor that appears
$ git push --force-with-lease

The following commits are merge commits:

fafe478

This comment has been minimized.

Sign in to view


          Added documentation to certain types

FractalFir force-pushed the master branch from 0555353 to 0476408 Compare

April 11, 2024 19:28

rustbot removed the S-waiting-on-author label

Contributor Author

FractalFir commented Apr 11, 2024

Should be OK to merge now. @rustbot review

Contributor

apiraino commented May 2, 2024

@FractalFir I see perhaps a few tiny nits around still pending, is it correct? Example here and here. thanks

@rustbot author

rustbot added S-waiting-on-author and removed S-waiting-on-review labels

Contributor Author

FractalFir commented May 14, 2024

I am not happy with this PR (some of what I wrote should be reworded). I think closing it and starting from scratch will be a better option.

FractalFir closed this

apiraino removed the S-waiting-on-author label

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

has-merge-commits T-compiler