This is an archive of the discontinued LLVM Phabricator instance.

[clang] Remove rdar links; NFC
ClosedPublic

Authored by aaron.ballman on Aug 16 2023, 4:54 AM.

Details

Summary

We have a new policy in place making links to private resources something we try to avoid in source and test files. Normally, we'd organically switch to the new policy rather than make a sweeping change across a project. However, Clang is in a somewhat special circumstance currently: recently, I've had several new contributors run into rdar links around test code which their patch was changing the behavior of. This turns out to be a surprisingly bad experience, especially for newer folks, for a handful of reasons: not understanding what the link is and feeling intimidated by it, wondering whether their changes are actually breaking something important to a downstream in some way, having to hunt down strangers not involved with the patch to impose on them for help, accidental pressure from asking for potentially private IP to be made public, etc. Because folks run into these links entirely by chance (through fixing bugs or working on new features), there's not really a set of problematic links to focus on -- all of the links have basically the same potential for causing these problems. As a result, this is an omnibus patch to remove all such links.

This was not a mechanical change; it was done by manually searching for rdar, radar, radr, and other variants to find all the various problematic links. From there, I tried to retain or reword the surrounding comments so that we would lose as little context as possible. However, because most links were just a plain link with no supporting context, the majority of the changes are simple removals.

I'm putting this up as a patch for review and adding reviewers from Apple in case someone wants to go through the removed links to see if there are situations where adding more supporting comments would be warranted. For example, there were a few links removed from source code instead of test files; perhaps augmenting those comments would be of benefit? However, I do not expect every one of these links to be replaced with comments as part of this review. (If someone has different expectations, let's work together on a practical way to proceed.)

Diff Detail

Event Timeline

aaron.ballman created this revision.Aug 16 2023, 4:54 AM
Herald added a project: Restricted Project. · View Herald Transcript
aaron.ballman requested review of this revision.Aug 16 2023, 4:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 16 2023, 4:54 AM
Herald added a subscriber: wangpc. · View Herald Transcript

Having worked with phabricator for a long time, this is the first time I've seen it "pull a github" where it just doesn't want to show you the content of anything in the review. Ugh, sorry for that! You can click on Changeset List above to see the list of changed files, and you can click on individual files to see changes and leave comments, which is pretty awful because of the amount of clicking involved but does work. I could split it up by top-level directory, but then we'd have approx 30 reviews to cover which feels like more effort than an NFC change to comments is worth. I'd recommend we try to advance with this review as best we can (I'm assuming there's only some subset of files people are interested in trying to retain comments for, so thinking that clicking around won't be too awful), but if others have a suggestion on a better way that isn't overly onerous, I'm happy to consider it.

NoQ added a comment.Aug 16 2023, 1:06 PM

Uh-oh looks like I missed 3 links that were spelled as radar://. I confirm that they can be safely removed as I looked into those internal bug reports and found no useful information to add.

I'll take a brief look at the links in static analyzer tests later today or tomorrow.

Thank you for your hard work @aaron.ballman, it is valuable to have everybody included equally in LLVM development.

Uh-oh looks like I missed 3 links that were spelled as radar://. I confirm that they can be safely removed as I looked into those internal bug reports and found no useful information to add.

Thank you! And yeah, I found *so many* fascinating ways to spell out the links. :-D

I'll take a brief look at the links in static analyzer tests later today or tomorrow.

Thank you!

Thank you for your hard work @aaron.ballman, it is valuable to have everybody included equally in LLVM development.

You're very welcome -- thank you for the help with this process!

I had a cursory look and the handful of radar links I opened and read through didn't have any context that wasn't already in the tests.

I had a cursory look and the handful of radar links I opened and read through didn't have any context that wasn't already in the tests.

Thank you for the quick look! FWIW, I'm happy to leave this review open for longer if you'd like to take a deeper dive through the changes. Alternatively, I'm happy to land the changes and we can add additional comments via post-commit review.

Nothing is on fire where we need to land this ASAP, I just want to avoid a review that goes stale and is painful to keep rebased given its size.

zaks.anna accepted this revision.Aug 28 2023, 8:55 AM

Looks like @NoQ wetted the remaining code changes. The rest LGTM.

Thank you for preparing the patch!

This revision is now accepted and ready to land.Aug 28 2023, 8:55 AM

Looks like @NoQ wetted the remaining code changes. The rest LGTM.

Thank you for preparing the patch!

Thank you for the help with review (to both you and @NoQ)! I've landed the changes but anyone who wants to add more context from any of the former links is encouraged to do so with NFC commits as they see fit.

cor3ntin closed this revision.Sep 21 2023, 12:49 AM
cor3ntin added a subscriber: cor3ntin.

This was landed