[PATCH] D61989: [clang-tidy] enable modernize-concat-nested-namespaces on header files

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

Could you please run this check over LLVM and give a short report of your finding? I would imagine that there is a lot of duplication, given the include-heavy nature of big c++ code bases.



================
Comment at: clang-tools-extra/clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp:70
+  if (!utils::parseHeaderFileExtensions(RawStringHeaderFileExtensions,
+                                        HeaderFileExtensions, ','))
+    llvm::errs() << "Invalid header file extension: "
----------------
Can we use the semicolon instead for the options list? Other options in checks use that character for separating lists instead and I would like to go for consistency.

What happens for the error case here? Is the default list used?


================
Comment at: clang-tools-extra/clang-tidy/modernize/ConcatNestedNamespacesCheck.h:23
+///   - `HeaderFileExtensions`: a comma-separated list of filename extensions of
+///     header files (The filename extensions should not contain "." prefix).
+///     "h,hh,hpp,hxx" by default.
----------------
` prefix), "h,hh,hpp,hxx" by default.` (Note the comma after paren)


================
Comment at: clang-tools-extra/clang-tidy/modernize/ConcatNestedNamespacesCheck.h:25
+///     "h,hh,hpp,hxx" by default.
+///     For extension-less header files, using an empty string or leaving an
+///     empty string between "," if there are other filename extensions.
----------------
Maybe `To enable extension-less header files use an empty string or leave an empty section between to commas like in "h,hxx,,hpp".`?
I think the sentence reads a bit weird right now.


================
Comment at: clang-tools-extra/docs/ReleaseNotes.rst:143
+  <clang-tidy/checks/modernize-concat-nested-namespaces>` now supports
+  `HeaderFileExtensions` option and issues warnings for transitively included
+  header files that passes the header filter.
----------------
I think the sentence is a bit off.

- `now supports _the_ ...`
- `header files that _pass_ the header filter`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61989