[PATCH] clang-tidy documentation redirects

[aaron.ballman]

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.

[alexfh]

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]

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]

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

6

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

[alexfh]

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]

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]

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.

[aaron.ballman]

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]

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

7

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.

[aaron.ballman]

Ping

[alexfh]

Sorry for the delay and thanks for pinging me.

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

7

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]

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

7

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]

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

[alexfh]

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

8

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

add_new_check.py9 KBDownload

):

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]

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

8

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

[aaron.ballman]

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

[alexfh]

LG. Thank you!

[aaron.ballman]

Thanks! I've commit in r257347.

clang-tidy/add_new_check.py

 
 # 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()
+  doc_files = filter(
+      lambda s: s.endswith('.rst') and s != 'list.rst',
+      os.listdir(docs_dir))
+  doc_files.sort()
 
-  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()
+  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:
     for line in lines:

docs/clang-tidy/checks/cert-dcl03-c.rst

+.. title:: clang-tidy - cert-dcl03-c
+.. meta::
+   :http-equiv=refresh: 5;URL=misc-static-assert.html
+
+cert-dcl03-c
+============
+
+The cert-dcl03-c checker is an alias, please see
+`misc-static-assert <misc-static-assert.html>`_ for more information.

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

+.. title:: clang-tidy - cert-dcl54-cpp
+.. meta::
+   :http-equiv=refresh: 5;URL=misc-new-delete-overloads.html
+
+cert-dcl54-cpp
+==============
+
+The cert-dcl54-cpp checker is an alias, please see
+`misc-new-delete-overloads <misc-new-delete-overloads.html>`_ for more
+information.

docs/clang-tidy/checks/cert-dcl59-cpp.rst

+.. title:: clang-tidy - cert-dcl59-cpp
+.. meta::
+   :http-equiv=refresh: 5;URL=google-build-namespaces.html
+
+cert-dcl59-cpp
+==============
+
+The cert-dcl59-cpp checker is an alias, please see
+`google-build-namespaces <google-build-namespaces.html>`_ for more information.

docs/clang-tidy/checks/cert-err61-cpp.rst

+.. title:: clang-tidy - cert-err61-cpp
+.. meta::
+   :http-equiv=refresh: 5;URL=misc-throw-by-value-catch-by-reference.html
+
+cert-err61-cpp
+==============
+
+The cert-err61-cpp checker is an alias, please see
+`misc-throw-by-value-catch-by-reference <misc-throw-by-value-catch-by-reference.html>`_
+for more information.

docs/clang-tidy/checks/cert-fio38-c.rst

+.. title:: clang-tidy - cert-fio38-c
+.. meta::
+   :http-equiv=refresh: 5;URL=misc-non-copyable-objects.html
+
+cert-fio38-c
+============
+
+The cert-fio38-c checker is an alias, please see
+`misc-non-copyable-objects <misc-non-copyable-objects.html>`_ for more
+information.

docs/clang-tidy/checks/cert-oop11-cpp.rst

+.. title:: clang-tidy - cert-oop11-cpp
+.. meta::
+   :http-equiv=refresh: 5;URL=misc-move-constructor-init.html
+
+cert-oop11-cpp
+==============
+
+The cert-oop11-cpp checker is an alias, please see
+`misc-move-constructor-init <misc-move-constructor-init.html>`_ for more
+information.

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

 google-build-namespaces
 =======================
 
+"cert-dcl59-cpp" redirects here as an alias for this checker.
 
 Finds anonymous namespaces in headers.
 

docs/clang-tidy/checks/list.rst

 =========================
 
 .. toctree::   
+   cert-dcl03-c (redirects to misc-static-assert) <cert-dcl03-c>
    cert-dcl50-cpp
+   cert-dcl54-cpp (redirects to misc-new-delete-overloads) <cert-dcl54-cpp>
+   cert-dcl59-cpp (redirects to google-build-namespaces) <cert-dcl59-cpp>
    cert-err52-cpp
    cert-err58-cpp
    cert-err60-cpp
+   cert-err61-cpp (redirects to misc-throw-by-value-catch-by-reference) <cert-err61-cpp>
+   cert-fio38-c (redirects to misc-non-copyable-objects) <cert-fio38-c>
+   cert-oop11-cpp (redirects to misc-move-constructor-init) <cert-oop11-cpp>
    cppcoreguidelines-pro-bounds-array-to-pointer-decay
    cppcoreguidelines-pro-bounds-constant-array-index
    cppcoreguidelines-pro-bounds-pointer-arithmetic

docs/clang-tidy/checks/misc-move-constructor-init.rst

 misc-move-constructor-init
 ==========================
 
+"cert-oop11-cpp" redirects here as an alias for this checker.
 
 The check flags user-defined move constructors that have a ctor-initializer
 initializing a member or base class through a copy constructor instead of a

docs/clang-tidy/checks/misc-new-delete-overloads.rst

 misc-new-delete-overloads
 =========================
 
+"cert-dcl54-cpp" redirects here as an alias for this checker.
+
 The check flags overloaded operator new() and operator delete() functions that
 do not have a corresponding free store function defined within the same scope.
 For instance, the check will flag a class implementation of a non-placement

docs/clang-tidy/checks/misc-non-copyable-objects.rst

 misc-non-copyable-objects
 =========================
 
+"cert-fio38-c" redirects here as an alias for this checker.
+
 The check flags dereferences and non-pointer declarations of objects that are
 not meant to be passed by value, such as C FILE objects or POSIX
 pthread_mutex_t objects.

docs/clang-tidy/checks/misc-static-assert.rst

 misc-static-assert
 ==================
 
+"cert-dcl03-c" redirects here as an alias for this checker.
 
 Replaces ``assert()`` with ``static_assert()`` if the condition is evaluatable
 at compile time.

docs/clang-tidy/checks/misc-throw-by-value-catch-by-reference.rst

 misc-throw-by-value-catch-by-reference
 ======================================
 
+"cert-err61-cpp" redirects here as an alias for this checker.
+
 Finds violations of the rule "Throw by value, catch by reference" presented for example in "C++ Coding Standards" by H. Sutter and A. Alexandrescu. This check also has the option to find violations of the rule "Throw anonymous temporaries" (https://www.securecoding.cert.org/confluence/display/cplusplus/ERR09-CPP.+Throw+anonymous+temporaries). The option is named "CheckThrowTemporaries" and it's on by default.
 
 Exceptions: