[PATCH] D66505: Make add_new_check.py's insertion of registerCheck<> more closely match the sort order

[Daniel Sanders via Phabricator via cfe-commits]

dsanders updated this revision to Diff 218010.
dsanders added a comment.

Sort on the check name string


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66505

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
@@ -165,31 +165,50 @@
 header_added = False
 header_found = False
 check_added = False
+check_fq_name = module + '-' + check_name
 check_decl = ('CheckFactories.registerCheck<' + check_name_camel +
-  '>(\n"' + module + '-' + check_name + '");\n')
-
-for line in lines:
-  if not header_added:
-match = re.search('#include "(.*)"', line)
-if match:
-  header_found = True
-  if match.group(1) > check_name_camel:
+  '>(\n"' + check_fq_name + '");\n')
+
+lines = iter(lines)
+try:
+  while True:
+line = lines.next()
+if not header_added:
+  match = re.search('#include "(.*)"', line)
+  if match:
+header_found = True
+if match.group(1) > check_name_camel:
+  header_added = True
+  f.write('#include "' + check_name_camel + '.h"\n')
+  elif header_found:
 header_added = True
 f.write('#include "' + check_name_camel + '.h"\n')
-elif header_found:
-  header_added = True
-  f.write('#include "' + check_name_camel + '.h"\n')
-
-  if not check_added:
-if line.strip() == '}':
-  check_added = True
-  f.write(check_decl)
-else:
-  match = re.search('registerCheck<(.*)>', line)
-  if match and match.group(1) > check_name_camel:
+
+if not check_added:
+  if line.strip() == '}':
 check_added = True
 f.write(check_decl)
-  f.write(line)
+  else:
+match = re.search('registerCheck<(.*)> *\( *(?:"([^"]*)")?', line)
+last_line = None
+if match:
+  current_check_name = match.group(2)
+  if current_check_name is None:
+# If we didn't find the check name on this line, look on the
+# next one
+last_line = line
+line = lines.next()
+match = re.search(' *"([^"]*)"', line)
+if match:
+  current_check_name = match.group(1)
+  if current_check_name > check_fq_name:
+check_added = True
+f.write(check_decl)
+  if last_line:
+f.write(last_line)
+f.write(line)
+except StopIteration:
+  pass
 
 
 # Adds a release notes entry.


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
@@ -165,31 +165,50 @@
 header_added = False
 header_found = False
 check_added = False
+check_fq_name = module + '-' + check_name
 check_decl = ('CheckFactories.registerCheck<' + check_name_camel +
-  '>(\n"' + module + '-' + check_name + '");\n')
-
-for line in lines:
-  if not header_added:
-match = re.search('#include "(.*)"', line)
-if match:
-  header_found = True
-  if match.group(1) > check_name_camel:
+  '>(\n"' + check_fq_name + '");\n')
+
+lines = iter(lines)
+try:
+  while True:
+line = lines.next()
+if not header_added:
+  match = re.search('#include "(.*)"', line)
+  if match:
+header_found = True
+if match.group(1) > check_name_camel:
+  header_added = True
+  f.write('#include "' + check_name_camel + '.h"\n')
+  elif header_found:
 header_added = True
 f.write('#include "' + check_name_camel + '.h"\n')
-elif header_found:
-  header_added = True
-  f.write('#include "' + check_name_camel + '.h"\n')
-
-  if not check_added:
-if line.strip() == '}':
-  check_added = True
-  f.write(check_decl)
-else:
-  match = re.search('registerCheck<(.*)>', line)
-  if match and match.group(1) > check_name_camel:
+
+if not check_added:
+  if line.strip() == '}':
 check_added = True
 f.write(check_decl)
-  f.write(line)
+  else:
+match = re.search('registerCheck<(.*)> *\( *(?:"([^"]*)")?', line)
+last_line = None
+if match:
+  current_check_name = match.group(2)
+  

[PATCH] D66505: Make add_new_check.py's insertion of registerCheck<> more closely match the sort order

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

In D66505#1641776 , @aaron.ballman 
wrote:

> Given that the alphabetization we want really is based on the string literal, 
> would it make sense to look for that rather than the check name? Adding a few 
> more reviewers for a better mix of opinions.


Ideally, it would be nice to sort the list of registrations by the check name. 
But given that the call can span two lines, that may be slightly trickier to 
implement. It's worth giving it a try though ;)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66505



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

[PATCH] D66505: Make add_new_check.py's insertion of registerCheck<> more closely match the sort order

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added reviewers: alexfh, djasper, JonasToth.
aaron.ballman added a comment.

Given that the alphabetization we want really is based on the string literal, 
would it make sense to look for that rather than the check name? Adding a few 
more reviewers for a better mix of opinions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66505



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

[PATCH] D66505: Make add_new_check.py's insertion of registerCheck<> more closely match the sort order

[Daniel Sanders via Phabricator via cfe-commits]

dsanders created this revision.
dsanders added a reviewer: aaron.ballman.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Following on from review comments in D65919  
about the ordering
of the registerCheck<> calls. Ignore namespaces when finding the correct
insertion point to make the heuristic more accurate. The actual sort order
is based on the check name in the string but that isn't always on the same
line so the class name is used as a proxy.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D66505

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
@@ -185,7 +185,7 @@
   check_added = True
   f.write(check_decl)
 else:
-  match = re.search('registerCheck<(.*)>', line)
+  match = re.search('registerCheck<((.*::).*)>', line)
   if match and match.group(1) > check_name_camel:
 check_added = True
 f.write(check_decl)


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
@@ -185,7 +185,7 @@
   check_added = True
   f.write(check_decl)
 else:
-  match = re.search('registerCheck<(.*)>', line)
+  match = re.search('registerCheck<((.*::).*)>', line)
   if match and match.group(1) > check_name_camel:
 check_added = True
 f.write(check_decl)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits