[PATCH] D72421: [clang-tidy] Refresh the add_new_check.py now that we use a table + autofix

[Sylvestre Ledru via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGc348a2674b57: [clang-tidy] Refresh the add_new_check.py now 
that we use a table + autofix (authored by sylvestre.ledru).

Changed prior to commit:
  https://reviews.llvm.org/D72421?vs=236924&id=237178#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72421/new/

https://reviews.llvm.org/D72421

Files:
  clang-tools-extra/clang-tidy/add_new_check.py

Index: clang-tools-extra/clang-tidy/add_new_check.py
===
--- clang-tools-extra/clang-tidy/add_new_check.py
+++ clang-tools-extra/clang-tidy/add_new_check.py
@@ -294,37 +294,89 @@
 def update_checks_list(clang_tidy_path):
   docs_dir = os.path.join(clang_tidy_path, '../docs/clang-tidy/checks')
   filename = os.path.normpath(os.path.join(docs_dir, 'list.rst'))
+  # Read the content of the current list.rst file
   with open(filename, 'r') as f:
 lines = f.readlines()
+  # Get all existing docs
   doc_files = list(filter(lambda s: s.endswith('.rst') and s != 'list.rst',
  os.listdir(docs_dir)))
   doc_files.sort()
 
-  def format_link(doc_file):
+  def has_auto_fix(check_name):
+dirname, _, check_name = check_name.partition("-")
+
+checkerCode = os.path.join(dirname, get_camel_name(check_name)) + ".cpp"
+
+if not os.path.isfile(checkerCode):
+  return ""
+
+with open(checkerCode) as f:
+  code = f.read()
+  if 'FixItHint' in code or "ReplacementText" in code or "fixit" in code:
+# Some simple heuristics to figure out if a checker has an autofix or not.
+return ' "Yes"'
+return ""
+
+  def process_doc(doc_file):
 check_name = doc_file.replace('.rst', '')
+
 with open(os.path.join(docs_dir, doc_file), 'r') as doc:
   content = doc.read()
   match = re.search('.*:orphan:.*', content)
+
   if match:
-return ''
+# Orphan page, don't list it.
+return '', ''
 
   match = re.search('.*:http-equiv=refresh: \d+;URL=(.*).html.*',
 content)
-  if match:
-return '   %(check)s (redirects to %(target)s) <%(check)s>\n' % {
-'check': check_name,
-'target': match.group(1)
-}
-  return '   %s\n' % check_name
+  # Is it a redirect?
+  return check_name, match
+
+  def format_link(doc_file):
+check_name, match = process_doc(doc_file)
+if not match and check_name:
+  return '   `%(check)s <%(check)s.html>`_,%(autofix)s\n' % {
+'check': check_name,
+'autofix': has_auto_fix(check_name)
+  }
+else:
+  return ''
+
+  def format_link_alias(doc_file):
+check_name, match = process_doc(doc_file)
+if match and check_name:
+  if match.group(1) == 'https://clang.llvm.org/docs/analyzer/checkers':
+title_redirect = 'Clang Static Analyzer'
+  else:
+title_redirect = match.group(1)
+  # The checker is just a redirect.
+  return '   `%(check)s <%(check)s.html>`_, `%(title)s <%(target)s.html>`_,%(autofix)s\n' % {
+'check': check_name,
+'target': match.group(1),
+'title': title_redirect,
+'autofix': has_auto_fix(match.group(1))
+  }
+return ''
 
   checks = map(format_link, doc_files)
+  checks_alias = map(format_link_alias, doc_files)
 
   print('Updating %s...' % filename)
   with open(filename, 'w') as f:
 for line in lines:
   f.write(line)
-  if line.startswith('.. toctree::'):
+  if line.strip() == ".. csv-table::":
+# We dump the checkers
+f.write('   :header: "Name", "Offers fixes"\n')
+f.write('   :widths: 50, 20\n\n')
 f.writelines(checks)
+# and the aliases
+f.write('\n\n')
+f.write('.. csv-table:: Aliases..\n')
+f.write('   :header: "Name", "Redirect", "Offers fixes"\n')
+f.write('   :widths: 50, 50, 10\n\n')
+f.writelines(checks_alias)
 break
 
 
@@ -345,6 +397,11 @@
'underline': '=' * len(check_name_dashes)})
 
 
+def get_camel_name(check_name):
+  return ''.join(map(lambda elem: elem.capitalize(),
+ check_name.split('-'))) + 'Check'
+
+
 def main():
   language_to_extension = {
   'c': 'c',
@@ -384,13 +441,11 @@
 
   module = args.module
   check_name = args.check
-
+  check_name_camel = get_camel_name(check_name)
   if check_name.startswith(module):
 print('Check name "%s" must not start with the module "%s". Exiting.' % (
 check_name, module))
 return
-  check_name_camel = ''.join(map(lambda elem: elem.capitalize(),
- check_name.split('-'))) + 'Check'
   clang_tidy_path = os.path.dirname(sys.argv[0])
   module_path = os.path.join(clang_tidy_path, module)
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailma

[PATCH] D72421: [clang-tidy] Refresh the add_new_check.py now that we use a table + autofix

[Sylvestre Ledru via Phabricator via cfe-commits]

sylvestre.ledru marked 5 inline comments as done.
sylvestre.ledru added inline comments.



Comment at: clang-tools-extra/clang-tidy/add_new_check.py:372
+f.write('   :header: "Name", "Offers fixes"\n')
+f.write('   :widths: 50, 20\n\n')
 f.writelines(checks)

alexfh wrote:
> Is there a way to let sphinx find out column widths itself?
TIL, sphinx doesn't need this indeed!



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72421/new/

https://reviews.llvm.org/D72421



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72421: [clang-tidy] Refresh the add_new_check.py now that we use a table + autofix

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh accepted this revision.
alexfh added a comment.
This revision is now accepted and ready to land.

Awesome! Thanks for updating the script! A few nits, otherwise LG, if this 
works.




Comment at: clang-tools-extra/clang-tidy/add_new_check.py:308
+
+checkerCode = os.path.join(dirname,get_camel_name(check_name)) + ".cpp"
+

nit: Space after the comma.



Comment at: clang-tools-extra/clang-tidy/add_new_check.py:316
+  if 'FixItHint' in code or "ReplacementText" in code or "fixit" in code:
+# Some stupid heuristics to figure out if a checker has an autofix or 
not
+return ' "Yes"'

nit: They are not completely stupid, they are just simple and naive ;)
Also, please add a trailing period.



Comment at: clang-tools-extra/clang-tidy/add_new_check.py:328
   if match:
-return ''
+# Orphan page, don't list it
+return '', ''

Trailing period.



Comment at: clang-tools-extra/clang-tidy/add_new_check.py:350
+  if match.group(1) == 'https://clang.llvm.org/docs/analyzer/checkers':
+titleRedirect = 'Clang Static Analyzer'
+  else:

nit: The script uses a_different_naming_style.



Comment at: clang-tools-extra/clang-tidy/add_new_check.py:353
+titleRedirect = match.group(1)
+  # The checker is just a redirect
+  return '   `%(check)s <%(check)s.html>`_, `%(title)s 
<%(target)s.html>`_,%(autofix)s\n' % {

Trailing period.



Comment at: clang-tools-extra/clang-tidy/add_new_check.py:372
+f.write('   :header: "Name", "Offers fixes"\n')
+f.write('   :widths: 50, 20\n\n')
 f.writelines(checks)

Is there a way to let sphinx find out column widths itself?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72421/new/

https://reviews.llvm.org/D72421



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72421: [clang-tidy] Refresh the add_new_check.py now that we use a table + autofix

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

In D72421#1811544 , @sylvestre.ledru 
wrote:

> > Thanks for this, Also how would you feel about adding a parameter to the 
> > script that could create alias definitions in much the same way?
>
> Do you have more details about your expectation? I could have a look but in a 
> separate commit :)


An example says it best

  py add_new_check.py awesome make-unique --alias modernize make-unqiue

Running that would create a new check in the Awesome module called make-unique 
that is just an alias for modernize-make-unique. doing the necessary things 
like updating the list.rst, creating a blank doc that just redirects to 
modernize-make-unique and adding the 'New Alias check' for the release notes


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72421/new/

https://reviews.llvm.org/D72421



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72421: [clang-tidy] Refresh the add_new_check.py now that we use a table + autofix

[Sylvestre Ledru via Phabricator via cfe-commits]

sylvestre.ledru added a comment.

> Thanks for this, Also how would you feel about adding a parameter to the 
> script that could create alias definitions in much the same way?

Do you have more details about your expectation? I could have a look but in a 
separate commit :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72421/new/

https://reviews.llvm.org/D72421



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72421: [clang-tidy] Refresh the add_new_check.py now that we use a table + autofix

[Sylvestre Ledru via Phabricator via cfe-commits]

sylvestre.ledru added a comment.

It finds some stuff that I missed in the list :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72421/new/

https://reviews.llvm.org/D72421



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72421: [clang-tidy] Refresh the add_new_check.py now that we use a table + autofix

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

Thanks for this, Also how would you feel about adding a parameter to the script 
that could create alias definitions in much the same way?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72421/new/

https://reviews.llvm.org/D72421



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits