Page MenuHomePhabricator

[LLVM][llvm-cov] Inclusive language: rename option -name-whitelist to -name-allowlist
ClosedPublic

Authored by ZarkoCA on Oct 29 2021, 7:06 AM.

Details

Summary

Renamed the option for llvm-cov and changed variable names to use more
inclusive terms. Also changed the binary for the test.

Diff Detail

Event Timeline

ZarkoCA requested review of this revision.Oct 29 2021, 7:06 AM
ZarkoCA created this revision.
vsk added a comment.Nov 2 2021, 12:44 PM

Thanks for the patch. We usually go out of our way to not break llvm-cov workflows. I'd suggest:

  • Leaving the old -name-whitelist around for a release, but marked cl::Hidden. Using it should print a deprecation warning.
  • Adding a llvm-14 release note (llvm/docs/ReleaseNotes.txt) about the planned deprecation and removal of -name-whitelist. llvm-14 would ship both options.
  • Posting on llvm-dev (or a forum) about the change.
  • Removing -name-whitelist when llvm-15 branches.
ZarkoCA planned changes to this revision.Nov 5 2021, 9:06 AM

Thanks for the patch. We usually go out of our way to not break llvm-cov workflows. I'd suggest:

  • Leaving the old -name-whitelist around for a release, but marked cl::Hidden. Using it should print a deprecation warning.
  • Adding a llvm-14 release note (llvm/docs/ReleaseNotes.txt) about the planned deprecation and removal of -name-whitelist. llvm-14 would ship both options.
  • Posting on llvm-dev (or a forum) about the change.
  • Removing -name-whitelist when llvm-15 branches.

Thanks for the review, I will follow up with all of your suggestions and update the patch.

ZarkoCA updated this revision to Diff 385906.Nov 9 2021, 11:15 AM
  • now accept both options
  • bring back old tests

Thanks for the patch. We usually go out of our way to not break llvm-cov workflows. I'd suggest:

  • Leaving the old -name-whitelist around for a release, but marked cl::Hidden. Using it should print a deprecation warning.
  • Adding a llvm-14 release note (llvm/docs/ReleaseNotes.txt) about the planned deprecation and removal of -name-whitelist. llvm-14 would ship both options.
  • Posting on llvm-dev (or a forum) about the change.
  • Removing -name-whitelist when llvm-15 branches.

Again, thanks for the review and apologies if the patch was overzealous in removing the existing option.

I've posted an RFC for the option name change here: https://lists.llvm.org/pipermail/llvm-dev/2021-November/153632.html

alanphipps accepted this revision.Nov 23 2021, 11:40 AM

Thanks for putting this up! The changes appear to be straightforward, and it makes sense to deprecate the older option and still accept it until next release. LGTM.

This revision is now accepted and ready to land.Nov 23 2021, 11:40 AM
ZarkoCA updated this revision to Diff 390008.Nov 26 2021, 4:28 AM
ZarkoCA edited the summary of this revision. (Show Details)

-Fix binary file in test

Thanks for putting this up! The changes appear to be straightforward, and it makes sense to deprecate the older option and still accept it until next release. LGTM.

Thanks for the review and approval. There was an issue with the binary file in my previous diff. I've fixed and once I see this is passing pre-commit CI, I will go ahead and commit the patch.

Pushed 715d2dc126ee4aaf0cb1df8826c3f1072c952e66 to fix the docs buildbot break.