PiotrZSL added a comment.
Overall looks +- fine, these are last comments from me.
================
Comment at:
clang-tools-extra/clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp:127
+
+ for (size_t Index = 0; Index < Namespaces.size(); Index++) {
+ if (Namespaces[Index]->isNested())
----------------
this index looks reundant, you could use range-for
================
Comment at:
clang-tools-extra/clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp:144-153
+ for (SourceRange const &Front : Fronts)
+ DB << FixItHint::CreateRemoval(Front);
+ for (SourceRange const &Back : Backs)
+ DB << FixItHint::CreateRemoval(Back);
+ NamespaceString ConcatNameSpace = concatNamespaces();
+ DB << FixItHint::CreateReplacement(
+ SourceRange{ND->getBeginLoc(), ND->getLocation()}, ConcatNameSpace);
----------------
consider ordering removals from last to first...
personally i sometimes run into issues when we first removed something before
last change, but that could be already fixed, or we may not run into this issue
in this check
================
Comment at:
clang-tools-extra/clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp:88
+ StringRef TokText = getRawStringRef(TokRange, SM, LangOpts);
+ if (TokText != "// namespace " + ND->getNameAsString())
+ return DefaultSourceRange;
----------------
PiotrZSL wrote:
> what if it is //namespace ? or /* namespace XYZ */
ok, but it still can be things like `// namespace XYZ`
So instead adding single space, consider using some trim/strip.
but thats minor issues, side effect would be just an leftover namespace comment.
add some test with something like `// namespace XYZ - closing namespace` to
verify that some custom comments wont be removed, and make sure that
documentation mention this, that we remove only namespace comments that follow
some known standard.
you could also check for:
```
namespace C
{
} // C
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D147194/new/
https://reviews.llvm.org/D147194
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
