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

[Xiao Shi via Phabricator via cfe-commits]

shixiao marked 2 inline comments as done.
shixiao 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.

Sure. I'll run it and report back. By "a lot of duplication", do you mean duplication across multiple translation units (since the same header would be included in many TUs)? Not knowing when and how `XXXCheck::check()` is invoked, I'd expect this to be a problem on any check that reports diagnostics in header files, right?



================
Comment at: clang-tools-extra/clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp:70
+  if (!utils::parseHeaderFileExtensions(RawStringHeaderFileExtensions,
+                                        HeaderFileExtensions, ','))
+    llvm::errs() << "Invalid header file extension: "
----------------
JonasToth wrote:
> 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?
@JonasToth, I followed the `HeaderFileExtensions` options for `misc-definitions-in-headers`, `google-build-namespaces`, and `google-global-names-in-headers` checks. (They have different defaults... but the format is the same). I thought consistency with the same option in other checks would be preferable and less confusing.

---

Currently, the error case results in using an empty list as header extensions. I could change it to use the default list if you prefer.


================
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.
----------------
JonasToth wrote:
> I think the sentence is a bit off.
> 
> - `now supports _the_ ...`
> - `header files that _pass_ the header filter`
Ah, thanks! I'll make corresponding edits here and above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61989