[clang-tidy] insert release notes for new checkers alphabetically

[MyDeveloperDay]

Summary

Almost all code review comments on new checkers D55433: [clang-tidy] Adding a new modernize use nodiscard checker D48866: [clang-tidy] Add incorrect-pointer-cast checker D54349: [clang-tidy] new check 'readability-redundant-preprocessor' seem to ask for the release notes to be added alphabetically, plus I've seen commits by @Eugene.Zelenko reordering the lists

Make add_new_check.py add those release notes alphabetically based on checker name

If include-fixer section is seen add it at the end

Minor change in the message format to prevent double newlines added before the checker.

Do the tools themselves have unit tests? (sorry new to this game)

• Tested adding new checker at the beginning
• Tested on adding new checker in the middle
• Tested on empty ReleasesNotes.rst (as we would see after RC)

[Eugene.Zelenko]

Thank you for this fix!

[MyDeveloperDay]

In D55508#1325316, @Eugene.Zelenko wrote:

Thank you for this fix!

You are welcome, I was reviewing other revisions to try and get up to speed, and I saw you giving someone else the same comment you gave mine! I'm no python expert so this could possible be done a better way, but I hope this at least give some relief.

[JonasToth]

clang-tidy/add_new_check.py

213 ↗

(On Diff #177491)

I think it would be better to compile the regular expression and match with the compiled version.

218 ↗

(On Diff #177491)

Please add whitespace around =

233 ↗

(On Diff #177491)

what do you mean with the &? Is this intended as bit-operator?

a if header_found and add_note_here: is better for logical expressions in python.

[MyDeveloperDay]

Fix review comments

[JonasToth]

I think this patch is fine, AFAIK these utility scripts are not tested directly but are just adjusted if they dont work as expected :)

Did you test it with a fake new-check? If it does what we expect its fine :)

[MyDeveloperDay]

In D55508#1325670, @JonasToth wrote:

I think this patch is fine, AFAIK these utility scripts are not tested directly but are just adjusted if they dont work as expected :)

Did you test it with a fake new-check? If it does what we expect its fine :)

I tested it with both a couple of new fake checks, but also removed everything back to how the docs/ReleaseNotes.rst would be after the release branches, i.e. with just "The improvements are..."

[JonasToth]

LGTM. Do you have commit access?

[MyDeveloperDay]

In D55508#1325758, @JonasToth wrote:

LGTM. Do you have commit access?

I do not I'm afraid

[JonasToth]

In D55508#1325834, @MyDeveloperDay wrote:

In D55508#1325758, @JonasToth wrote:

LGTM. Do you have commit access?

I do not I'm afraid

I will commit for you.

[Eugene.Zelenko]

By the word, will be good idea to have script which check alphabetical order and use it during build. Sometimes alphabetical order may be violated after merge with trunk.

[JonasToth]

In D55508#1325960, @Eugene.Zelenko wrote:

By the word, will be good idea to have script which check alphabetical order and use it during build. Sometimes alphabetical order may be violated after merge with trunk.

I think that should be in the validate-check.py proposed in the other patch. If we just add it to the normal testing/documentation generation it would be best. I think it is like a unit-test in spirit.

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

 # 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:
     lines = f.readlines()
 
+  lineMatcher = re.compile('Improvements to clang-tidy')
+  nextSectionMatcher = re.compile('Improvements to include-fixer')
+  checkerMatcher = re.compile('- New :doc:`(.*)')
+
   print('Updating %s...' % filename)
   with open(filename, 'w') as f:
     note_added = False
     header_found = False
+    next_header_found = False
+    add_note_here = False
 
     for line in lines:
       if not note_added:
-        match = re.search('Improvements to clang-tidy', line)
+        match = lineMatcher.match(line)
+        match_next = nextSectionMatcher.match(line)
+        match_checker = checkerMatcher.match(line)
+        if match_checker:
+          last_checker = match_checker.group(1)
+          if last_checker > check_name_dashes:
+            add_note_here = True
+
+        if match_next:
+          next_header_found = True
+          add_note_here = True
+
         if match:
           header_found = True
-        elif header_found:
+          f.write(line)
+          continue
+
+        if line.startswith('----'):
+          f.write(line)
+          continue
+
+        if header_found and add_note_here:
           if not line.startswith('----'):
-            f.write("""
-- New :doc:`%s
+            f.write("""- New :doc:`%s
   <clang-tidy/checks/%s>` check.
 
   FIXME: add release notes.
+
 """ % (check_name_dashes, check_name_dashes))
             note_added = True
 
       f.write(line)
 
 
 # Adds a test for the check.
 def write_test(module_path, module, check_name, test_extension):