This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Organize test docs into subdirectories by module (NFC)
ClosedPublic

Authored by LegalizeAdulthood on May 26 2022, 12:18 PM.

Details

Summary
  • Rename doc files
  • Update add_new_check.py to handle doc subdirs
  • Improve add_new_check.py to preserve static analyzer doc anchors

Diff Detail

Event Timeline

LegalizeAdulthood requested review of this revision.May 26 2022, 12:18 PM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: sstefan1. · View Herald Transcript
  • Update add_new_check.py for the case when adding a new check, not just updating the docs
  • Rebase ReleaseNotes.rst

I know this is a rather huge diff, but the part that needs to be reviewed is the python script changes.

I've rebuilt the HTML docs with sphinx and tested all the manually modified links to verify that they still go to the correct page.

The existing Release Notes have been rebased as well.

Can I ask what the motivation is for this change?

You may want to check the actual header files for the checks. A lot of them have a comment saying.

/// For the user-facing documentation see:
/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-reserved-identifier.html

I'm no expert on the inner workings of search engines, but I feel this will temporarily mess up my go to way of finding out a checks documentation, just googling its name.
I wouldn't say its reason for blocking this, just my 0.02$.

clang-tools-extra/clang-tidy/add_new_check.py
86–87

Again, this comment would need to be updated to use the new directory structure/

Basically there are a crapton of files in a single directory, making for clutter.

Additionally, on Windows, TAB completion of filenames in cmd shells doesn't stop at the first non-unique character in a path, so it's essentially useless when a crapton of files all begin with the same prefix. If the files are grouped by module, then I can TAB complete on the module and usually the checks differ after a few characters in the check name once the module prefix is stripped, so typing a few characters and then TAB completes to the desired file or a few more TABs to cycle through the remaining matching files to get the one I want.

njames93 added a comment.EditedJun 8 2022, 9:55 PM

Gentle ping

My previous point about the links in the header files not being updated hasn't been addressed.

It would also be nice if there was a redirect that would dynamically translate the old links to new links. A regex like this would do the job, but I have no idea on the inner workings of sphinx in order to set this up: -clang-tidy/checks/(\w+)((-\w+)+) -> clang-tidy/checks/$1/$2

Gentle ping

My previous point about the links in the header files not being updated hasn't been addressed.

Ah yes, I got distracted and forgot about that. I will update the diff.

It would also be nice if there was a redirect that would dynamically translate the old links to new links.

You can do that with .htaccess, but I don't know if that's considered acceptable in clang documentation.

Leaving .rst pages in place for all the old locations just recreates the clutter instead of getting rid of it.

  • Update add_new_check.py to write out module subdir link in generated header
  • Update all links in check headers

It would also be nice if there was a redirect that would dynamically translate the old links to new links.

You can do that with .htaccess, but I don't know if that's considered acceptable in clang documentation.

FWIW, I don't have any idea if this is or isn't acceptable.

Leaving .rst pages in place for all the old locations just recreates the clutter instead of getting rid of it.

+1

I don't feel super comfortable reviewing the Python changes, but I spot checked the changes and they all seem reasonable to me. So this has my LG, but I'm not accepting the review because of the Python bit.

LegalizeAdulthood added a comment.EditedJun 9 2022, 2:11 PM

It would also be nice if there was a redirect that would dynamically translate the old links to new links.

You can do that with .htaccess, but I don't know if that's considered acceptable in clang documentation.

FWIW, I don't have any idea if this is or isn't acceptable.

Mucking around with an .htaccess file is very web server dependent. Surely this can't be the first time that a page of documentation changed names or locations? I'm betting that they didn't worry about putting in redirects and let search engines handle guiding users to the correct page.

It would also be nice if there was a redirect that would dynamically translate the old links to new links.

You can do that with .htaccess, but I don't know if that's considered acceptable in clang documentation.

FWIW, I don't have any idea if this is or isn't acceptable.

Mucking around with an .htaccess file is very web server dependent. Surely this can't be the first time that a page of documentation changed names or locations? I'm betting that they didn't worry about putting in redirects and let search engines handle guiding users to the correct page.

Search engines isn't really an issue, its more people who are using older versions of llvm that try and go to the documentation which turns out is a dead link.
Having said that, I still think the lack of a redirect shouldn't block this change.

Search engines isn't really an issue, its more people who are using older versions of llvm that try and go to the documentation which turns out is a dead link.
Having said that, I still think the lack of a redirect shouldn't block this change.

Aren't we archiving old versions of the documentation, though?

So if I know I'm using LLVM 3.8, I would consult the documentation for 3.8 not top-of-tree, and all the links would work correctly within the 3.8 documentation, adjusting for the base path.

Ping. All review comments should be addressed in the latest diff.

aaron.ballman accepted this revision.Jun 16 2022, 6:33 AM

Search engines isn't really an issue, its more people who are using older versions of llvm that try and go to the documentation which turns out is a dead link.
Having said that, I still think the lack of a redirect shouldn't block this change.

Aren't we archiving old versions of the documentation, though?

We do:

https://releases.llvm.org/12.0.0/tools/clang/tools/extra/docs/
https://releases.llvm.org/11.0.0/tools/clang/tools/extra/docs/
and so on.

Giving my LG more officially now to unblock this, but I'd still appreciate some extra eyes on the Python changes.

This revision is now accepted and ready to land.Jun 16 2022, 6:33 AM

Search engines isn't really an issue, its more people who are using older versions of llvm that try and go to the documentation which turns out is a dead link.
Having said that, I still think the lack of a redirect shouldn't block this change.

Aren't we archiving old versions of the documentation, though?

We do:

https://releases.llvm.org/12.0.0/tools/clang/tools/extra/docs/
https://releases.llvm.org/11.0.0/tools/clang/tools/extra/docs/
and so on.

Giving my LG more officially now to unblock this, but I'd still appreciate some extra eyes on the Python changes.

I tested by generating a new check and validating the links in the documentation as well as running --update-docs and checking that the updates were correct. So functionally, there should be no problem and I tried to follow the existing style of the file, so should be good on style nits as well.

This revision was landed with ongoing or failed builds.Jun 16 2022, 3:06 PM
This revision was automatically updated to reflect the committed changes.