This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] enable modernize-concat-nested-namespaces on header files
Needs ReviewPublic

Authored by shixiao on May 15 2019, 8:37 PM.

Details

Summary

For header files that pass the -header-filter, the
modernize-concat-nested-namespaces check does not generate the expected
warning and fix for un-concatenated namespaces. This diff fixes it.

https://bugs.llvm.org/show_bug.cgi?id=41670

This diff also enables the check for c++2a.

Test Plan:

Extended unit test

make check-clang-tools/fast

Manual testing previously:

====a.h===
namespace foo {
namespace bar {
namespace baz {
struct S {};
}
}
}
========

====a.cpp====

namespace foo {
namespace bar {
namespace baz {
void foo(const S&) {}
}
}
}

int main () {
  return 0;
}
========

$ /clang-tidy -p sample/compile_commands.json -checks="modernize-concat-nested-namespaces" -header-filter="a.h" sample/a.cpp
  4 warnings generated.
  sample/a.h:5:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
  namespace foo {
  ^~~~~~~~~~~~~~~
  namespace foo::bar::baz
  sample/a.cpp:5:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
  namespace foo {
  ^~~~~~~~~~~~~~~
  namespace foo::bar::baz

Ran the check on the llvm code base:

./llvm-project/clang-tools-extra/clang-tidy/tool/run-clang-tidy.py -p \
llvm-project/llvm llvm-project/llvm -header-filter='llvm-project/llvm.*' \
-checks="-*,modernize-concat-nested-namespaces" -extra-arg='--std=c++17' \
-j 8

The entire output is 8.4M. Generated 33k warnings, among which 275 on .cpp
files, the rest on 447 different headers. Below is a list of the number of
duplicate warnings for a particular header:

... ...
104 llvm/include/llvm/BinaryFormat/Wasm.h
106 llvm/include/llvm/Support/EndianStream.h
117 llvm/include/llvm/Support/Path.h
120 llvm/include/llvm/BinaryFormat/COFF.h
140 llvm/include/llvm/DebugInfo/CodeView/TypeRecord.h
142 llvm/include/llvm/BinaryFormat/ELF.h
146 llvm/include/llvm/DebugInfo/CodeView/RecordSerialization.h
162 llvm/include/llvm/CodeGen/MIRYamlMapping.h
163 llvm/include/llvm/DebugInfo/CodeView/CodeView.h
168 llvm/include/llvm/BinaryFormat/MachO.h
172 llvm/lib/Target/AMDGPU/SIDefines.h
225 llvm/include/llvm/Object/SymbolicFile.h
250 llvm/include/llvm/Object/Error.h
292 llvm/include/llvm/Support/AArch64TargetParser.h
293 llvm/include/llvm/Support/ARMTargetParser.h
302 llvm/include/llvm/DebugInfo/CodeView/CodeViewError.h
321 llvm/include/llvm/Support/YAMLTraits.h
347 llvm/utils/unittest/googletest/include/gtest/gtest-printers.h
347 llvm/utils/unittest/googletest/include/gtest/internal/gtest-death-test-internal.h
347 llvm/utils/unittest/googletest/include/gtest/internal/gtest-filepath.h
347 llvm/utils/unittest/googletest/include/gtest/internal/gtest-linked_ptr.h
347 llvm/utils/unittest/googletest/include/gtest/internal/gtest-string.h
384 llvm/include/llvm/Support/CFGUpdate.h
498 llvm/include/llvm/CodeGen/RuntimeLibcalls.h
504 llvm/include/llvm/CodeGen/TargetCallingConv.h
514 llvm/include/llvm/CodeGen/ISDOpcodes.h
530 llvm/include/llvm/CodeGen/MachineOutliner.h
620 llvm/include/llvm/CodeGen/PBQPRAConstraint.h
863 llvm/include/llvm/Support/FileSystem.h
938 llvm/include/llvm/Support/Mutex.h
1389 llvm/include/llvm/Support/RWMutex.h
1393 llvm/include/llvm/IR/CallingConv.h
2037 llvm/include/llvm/Support/Endian.h
2308 llvm/include/llvm/Support/Host.h
2380 llvm/include/llvm/Support/raw_ostream.h
2465 llvm/include/llvm/Support/SwapByteOrder.h
6903 llvm/include/llvm/ADT/Hashing.h

This seems excessive, but the --header-filter option (along with
--fix) would make this more manageable--a user could go through
a large code base directory by directory; alternatively, using the
--fix option would apply the fix and avoid a duplicate warning at the
same spot.

Event Timeline

shixiao created this revision.May 15 2019, 8:37 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 15 2019, 8:37 PM
shixiao edited the summary of this revision. (Show Details)May 15 2019, 8:38 PM
shixiao added reviewers: wgml, hokein, aaron.ballman, alexfh.

Will be good idea to mention new option in the Release Notes.

clang-tools-extra/docs/clang-tidy/checks/modernize-concat-nested-namespaces.rst
56

Please use single back-ticks for highlighting option values. Same in other places.

Eugene.Zelenko added a project: Restricted Project.

Hi shixiao,

thank you for taking a look at this. Please extend the unit-tests (clang-tools-extra/test/clang-tidy) for this check, your revision description already includes a good start.

clang-tools-extra/clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp
69

please ellide braces for the single-stmt ifs.

shixiao updated this revision to Diff 199940.May 16 2019, 5:04 PM
shixiao marked 2 inline comments as done.
shixiao edited the summary of this revision. (Show Details)
shixiao removed a project: Restricted Project.
shixiao removed a subscriber: Eugene.Zelenko.

elide braces for single-stmt if-s, extend unit test

shixiao updated this revision to Diff 199942.May 16 2019, 5:21 PM
shixiao added a subscriber: Eugene.Zelenko.

edit doc, add note in release notes, extend unit test to test transitive header includes

shixiao marked 2 inline comments as done.May 20 2019, 10:12 AM

@JonasToth, does this look reasonable to you? Thanks!

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.

clang-tools-extra/clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp
70

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?

clang-tools-extra/clang-tidy/modernize/ConcatNestedNamespacesCheck.h
23

prefix), "h,hh,hpp,hxx" by default. (Note the comma after paren)

25

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.

clang-tools-extra/docs/ReleaseNotes.rst
168

I think the sentence is a bit off.

  • now supports _the_ ...
  • header files that _pass_ the header filter
shixiao marked 2 inline comments as done.May 28 2019, 7:05 AM

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?

clang-tools-extra/clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp
70

@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.

clang-tools-extra/docs/ReleaseNotes.rst
168

Ah, thanks! I'll make corresponding edits here and above.

shixiao updated this revision to Diff 203275.Jun 5 2019, 6:47 PM

rebase, fix comments, enable for c++2a, add report for running the check on llvm

shixiao marked 3 inline comments as done.Jun 5 2019, 6:48 PM
shixiao updated this revision to Diff 203276.Jun 5 2019, 6:49 PM
shixiao edited the summary of this revision. (Show Details)

update commit summary

Could someone please take a look? I think I've addressed all of Jonas's comments above. Thank you!

alexfh added inline comments.Jul 15 2019, 3:13 AM
clang-tools-extra/test/clang-tidy/modernize-concat-nested-namespaces.h
2

Please put the header under Inputs/<check-name>/ directory to keep the tests directory a bit tidier. The header can then be renamed to something shorter, btw.

2

Is it important to use #pragma once in this file? Can we use regular include guards instead?

shixiao marked an inline comment as done.Jul 18 2019, 6:10 AM
shixiao added inline comments.
clang-tools-extra/test/clang-tidy/modernize-concat-nested-namespaces.h
2

It's not--I added it out of habit. Any particular reason to use regular include guards vs. #pragma once?

re: mv headers--sure, will do.

aaron.ballman added inline comments.Jul 18 2019, 6:15 AM
clang-tools-extra/test/clang-tidy/modernize-concat-nested-namespaces.h
2

Regular include guards work reliably and portably. #pragma once is neither portable, nor works in correctly all circumstances even when it is supported (think about atrocities like including the same file over more than one network mount point).

Sorry for not responding to the review. I was very busy with other things and overlooked the mails :(

clang-tools-extra/clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp
82

We would need to update that line for each new standard, that won't happen :/
I think the opposite logic should be used here and blacklist known-not working standards (and C).

Are there any updates on this change? I see there's a comment that hasn't been addressed yet. Is that the only roadblock stopping this from getting merged?

Are there any updates on this change? I see there's a comment that hasn't been addressed yet. Is that the only roadblock stopping this from getting merged?

Tbh clang-tidy already handles disabling warnings for header files, so some of the code here isn't needed

@shixiao do you have plans to keep working on this diff? If not, do you have any objections if I send for review a similar diff that will eliminate the check Sources.isInMainFile and will rely on generic header filtration mechanism in clang-tidy?

We have faced the same issue on our codebase and would like to make this check works on headers too.

Tbh clang-tidy already handles disabling warnings for header files, so some of the code here isn't needed

+1 to use generic header filtration mechanism and avoid adding new option for this particular check. I tried to find the reason why Sources.isInMainFile was added but it looks like it was not even discussed during initial codereview D52136

Everyone please speak up if there is a good reason to limit this check to the main file only.

Everyone please speak up if there is a good reason to limit this check to the main file only.

The only possible reason I can see to limit to main file only is if the header is included in pre-c++17 and post c++17 translation units. Though that is a very niche use case IMO.