This is an archive of the discontinued LLVM Phabricator instance.

[PATCH] clang-tidy documentation redirects
ClosedPublic

Authored by aaron.ballman on Dec 28 2015, 9:31 AM.

Details

Reviewers
alexfh
sbenza
Summary

Some of our checkers are registered under multiple different check names. This works well in terms of command line arguments, but does not work well for documentation discoverability. This patch adds documentation for the CERT checks that automatically redirects to the canonical documentation for the checker. It also updates the canonical checkers to have a list of aliases under which the check can be enabled.

Currently, the patch does not add the checkers to list.rst, so they are only discoverable by using the checker name directly. This prevents cluttering up the list of checkers by only listing the canonical ones. However, for the sake of discoverability, we may want to include the duplicated checkers. I wasn't certain which direction we wanted to go, so recommendations are welcome here.

Diff Detail

Event Timeline

aaron.ballman retitled this revision from to [PATCH] clang-tidy documentation redirects.
aaron.ballman updated this object.
aaron.ballman added reviewers: alexfh, sbenza.
aaron.ballman added a subscriber: cfe-commits.
alexfh added inline comments.Dec 29 2015, 7:22 AM
docs/clang-tidy/checks/cert-dcl54-cpp.rst
6

Since this is not a deprecated/obsolete file, it might be a good idea to also add a textual redirection with native RST links for media not supporting HTTP redirects (e.g. PDF or whatever else can be generated from the RST files).

docs/clang-tidy/checks/google-build-namespaces.rst
6

nit: Missing trailing period.

Also, it might be a good idea to make this slightly more verbose, e.g. s/Aliases/This check is available under the following names/ or something like this.

aaron.ballman added inline comments.Dec 29 2015, 7:27 AM
docs/clang-tidy/checks/cert-dcl54-cpp.rst
6

So basically use a regular link instead of the meta redirect?

docs/clang-tidy/checks/google-build-namespaces.rst
6

Agreed.

alexfh added inline comments.Dec 29 2015, 7:55 AM
docs/clang-tidy/checks/cert-dcl54-cpp.rst
6

I'd use either a regular link, or both a link and the redirect.

alexfh added inline comments.Dec 29 2015, 8:03 AM
docs/clang-tidy/checks/cert-dcl54-cpp.rst
6

One interesting aspect to consider is the bingability of check names, e.g. http://www.bing.com/search?q=google-build-namespaces or http://www.google.com/search?q=misc-new-delete-overloads (apparently, this one doesn't work well with bing).

I'm not sure redirects will play well with it, but we can try. Inclusion of the checks in the index seems like the must though, if we want the checks to be searchable.

aaron.ballman added inline comments.Dec 29 2015, 8:30 AM
docs/clang-tidy/checks/cert-dcl54-cpp.rst
6

I think a link and the redirect make sense. Would a 5 second delay be a reasonable amount of time before the auto redirect?

Also, the more I think on it, the more I agree that we want the checks in the index file. I think it might be best to make the aliases visually distinct, though. So perhaps something like:

cert-dcl54-cpp (alias to misc-new-delete-overloads)

And then have the link go directly to misc-new-delete-overloads instead of making them click through or wait for the redirect?

alexfh added inline comments.Dec 29 2015, 8:56 AM
docs/clang-tidy/checks/cert-dcl54-cpp.rst
6

Let's try with links to aliases and a delayed redirect and then see how it works.

I think it might be best to make the aliases visually distinct, though. So perhaps something like:

cert-dcl54-cpp (alias to misc-new-delete-overloads)

We need to teach add_new_check.py to retain (or automatically generate) these comments then.

Changes in this revision:

  • Added the redirecting checkers to the list, with additional information that they will redirect to another page).
  • Changed the auto-redirect time to 5 seconds.
  • Added text and manual links to the redirecting page.
  • Changed the wording on the canonical page to mention what checkers redirect there as an alias.
aaron.ballman marked 2 inline comments as done.Jan 1 2016, 9:26 AM
aaron.ballman added inline comments.
docs/clang-tidy/checks/cert-dcl54-cpp.rst
6

We need to teach add_new_check.py to retain (or automatically generate) these comments then.

I have not touched add_new_check.py for this because add_new_check.py knows nothing about check aliases anyway. Whenever we update add_new_check.py to generate checker aliases directly, that would be a good time to do this work.

alexfh edited edge metadata.Jan 11 2016, 6:43 AM

Sorry for the delay and thanks for pinging me.

docs/clang-tidy/checks/cert-dcl54-cpp.rst
6

add_new_check.py should do something reasonable with list.rst right away, otherwise the (redirects to ...) comments will be lost on each run of the script, which will result in annoying manual work of reverting these changes. Do you need help implementing alias handling in the add_new_check.py script?

11

nit: Could you add newlines at the end of files to remove this annoying line?

aaron.ballman added inline comments.Jan 11 2016, 6:53 AM
docs/clang-tidy/checks/cert-dcl54-cpp.rst
6

Oye, that's a good point. If you don't mind tackling the Python side of things, that would be great.

11

Will do.

aaron.ballman edited edge metadata.

Adding newlines to the end of some files, per feedback.

aaron.ballman marked an inline comment as done.Jan 11 2016, 6:56 AM
alexfh added inline comments.Jan 11 2016, 7:25 AM
docs/clang-tidy/checks/cert-dcl54-cpp.rst
8

This seems to do what we need (full file is here

):

diff --git a/clang-tidy/add_new_check.py b/clang-tidy/add_new_check.py
index 6ecc89d..6000616 100755
--- a/clang-tidy/add_new_check.py
+++ b/clang-tidy/add_new_check.py
@@ -215,15 +215,25 @@ void awesome_f2();
 
 # Recreates the list of checks in the docs/clang-tidy/checks directory.
 def update_checks_list(module_path):
-  filename = os.path.normpath(
-      os.path.join(module_path, '../../docs/clang-tidy/checks/list.rst'))
+  docs_dir = os.path.join(module_path, '../../docs/clang-tidy/checks')
+  filename = os.path.normpath(os.path.join(docs_dir, 'list.rst'))
   with open(filename, 'r') as f:
     lines = f.readlines()
-
-  checks = map(lambda s: '   ' + s.replace('.rst', '\n'),
-               filter(lambda s: s.endswith('.rst') and s != 'list.rst',
-                      os.listdir(os.path.join(module_path, '../../docs/clang-tidy/checks'))))
-  checks.sort()
+  doc_files = filter(
+      lambda s: s.endswith('.rst') and s != 'list.rst',
+      os.listdir(docs_dir))
+  doc_files.sort()
+
+  def format_link(doc_file):
+    check_name = doc_file.replace('.rst', '')
+    with open(os.path.join(docs_dir, doc_file), 'r') as doc:
+      match = re.search('.*:http-equiv=refresh: \d+;URL=(.*).html.*', doc.read())
+      if match:
+        return '   %(check)s (redirects to %(target)s) <%(check)s>\n' % {
+            'check' : check_name, 'target' : match.group(1) }
+      return '   %s\n' % check_name
+
+  checks = map(format_link, doc_files)
 
   print('Updating %s...' % filename)
   with open(filename, 'wb') as f:

Can you include this in the patch?

alexfh added inline comments.Jan 11 2016, 7:26 AM
docs/clang-tidy/checks/cert-dcl54-cpp.rst
8

(and please re-generate the list using the script)

aaron.ballman marked 2 inline comments as done.

Adds the add_new_check.py changes from Alex, regenerates list.rst from the Python script.

alexfh accepted this revision.Jan 11 2016, 8:33 AM
alexfh edited edge metadata.

LG. Thank you!

This revision is now accepted and ready to land.Jan 11 2016, 8:33 AM
aaron.ballman closed this revision.Jan 11 2016, 8:52 AM

Thanks! I've commit in r257347.