Make add_new_check.py's insertion of registerCheck<> match the sort order

[dsanders]

Summary

Following on from review comments in D65919 about the ordering
of the registerCheck<> calls. Sort the calls based on the check name
string

[aaron.ballman]

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.

[alexfh]

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 ;)

[dsanders]

Sort on the check name string

[alexfh]

Mostly LG, if you've verified that this works. A couple of comments below.

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

192 ↗

(On Diff #218010)

Are there actually any registerCheck calls with spaces between the > and the (? Should we clang-format these instead of supporting them in the script?

193 ↗

(On Diff #218010)

I'd call this previous_line or prev_line. "Last" is a bit misleading here.

[aaron.ballman]

Generally LG to me as well

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

198 ↗

(On Diff #218010)

Missing a full stop at the end of the comment.

[dsanders]

In D66505#1652420, @alexfh wrote:

Mostly LG, if you've verified that this works. A couple of comments below.

I verified it using

./add_new_check.py llvm prefer-register-over-unsigned
./add_new_check.py llvm zzz
./add_new_check.py llvm nb
./add_new_check.py llvm iz
./add_new_check.py llvm a

which is enough to cover every insertion point in the LLVM module

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

192 ↗

(On Diff #218010)

Are there actually any registerCheck calls with spaces between the > and the (?

No, it was just trivial to be robust against it in case it does somehow happen. Likewise for the whitespace after the '(' but before the '"' or newline.

Should we clang-format these instead of supporting them in the script?

I think the script should stick to inserting the new code rather than correcting existing mistakes that somehow crept in. I do agree that people should generally clang-format their patches before commit though which will prevent it creeping in.

[dsanders]

• Full stop at end of comment
• last_line -> prev_line

[dsanders]

Missed one of the commands from my history that I used to verify it:

./add_new_check.py llvm i

[aaron.ballman]

LGTM, thank you for working on this!

clang-tools-extra/trunk/clang-tidy/add_new_check.py

def adapt_module(module_path, module, check_name, check_name_camel):
   with open(filename, 'r') as f:
     lines = f.readlines()
 
   print('Updating %s...' % filename)
   with open(filename, 'w') as f:
     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')
+                  '>(\n        "' + check_fq_name + '");\n')
 
-    for line in lines:
+    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')
 
         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:
+            match = re.search('registerCheck<(.*)> *\( *(?:"([^"]*)")?', line)
+            prev_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.
+                prev_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 prev_line:
+                f.write(prev_line)
         f.write(line)
+    except StopIteration:
+      pass
 
 
 # Adds a release notes entry.
 def add_release_notes(module_path, module, check_name):
   check_name_dashes = module + '-' + check_name
   filename = os.path.normpath(os.path.join(module_path,
                                            '../../docs/ReleaseNotes.rst'))
   with open(filename, 'r') as f: