[PATCH] D147194: [clang-tidy] fix concat-nest-namespace fix hint remove the macro

[Piotr Zegar via Phabricator via cfe-commits]

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