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

[docs] remove some out-of-date content in LLVM Programmer's Manual #74989

Merged
merged 1 commit into from
Dec 14, 2023

Conversation

yhgu2000
Copy link
Contributor

Remove the part about implicit conversion from an iterator to a pointer.

This part of the manual was written 14 years ago, in: 37027c3

There do exist a type casting operator in ilist then:

operator pointer() const {
return NodePtr;
}

But it has been remove since 2016: f197b1f

So I think it makes sense to remove this part to avoid mislead new contributors.

Comment on lines 2764 to 2766
..
The part about implicit conversion from an iterator to a pointer is out of
date and has been removed.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added these comments to let someone blame me in the future.

Copy link
Member

Choose a reason for hiding this comment

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

why not just delete it then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why not just delete it then?

When I went through the commit history to confirm the commit that removed operator pointer(), I used GitHub blame to find out who modified the code. I found that commits solely removing content are particularly difficult to locate, as they do not appear in GitHub blame. Therefore, I believe it's best for me to leave a comment here to ensure that someone in the future can blame me in case these revisions cause issues.

Do you think this is a good idea? Let me know your opinion.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's considerate, but I agree with @nickdesaulniers that we don't need to be so cautious. Let's just describe the current state accurately. (If we documented all the things we removed we wouldn't have space for the stuff that's still here!)

Remove the part about implicit conversion from an iterator to a pointer.

This part of the manual was written 14 years ago, in: llvm@37027c3

There do exist a type casting operator in `ilist` then: https://github.com/llvm/llvm-project/blob/37027c30ec526afe3bb571df6f8701bf0d322f22/llvm/include/llvm/ADT/ilist.h#L192-L194

But it has been remove since 2016: llvm@f197b1f

So I think it makes sense to remove this part to avoid mislead new contributors.
@yhgu2000
Copy link
Contributor Author

@dexonsmith @nickdesaulniers Advice taken!

Copy link
Collaborator

@dexonsmith dexonsmith left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@yhgu2000 yhgu2000 merged commit a2691e3 into llvm:main Dec 14, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation llvm Umbrella label for LLVM issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants