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

Piotr Zegar via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Mar 30 12:29:21 PDT 2023


PiotrZSL added a comment.

You changed half of this check logic, please take a look into those issues for this check:
https://github.com/llvm/llvm-project/issues/60051
https://github.com/llvm/llvm-project/issues/60048
https://github.com/llvm/llvm-project/issues/60035
https://github.com/llvm/llvm-project/issues/57530
https://github.com/llvm/llvm-project/issues/56022
https://github.com/llvm/llvm-project/issues/43351
https://github.com/llvm/llvm-project/issues/41015
Add issues that you fixing to commit message.

Also look into these patches:
https://reviews.llvm.org/D61989
https://reviews.llvm.org/D141787

Update release notes (add info about fixes for this check).

Would be good to run it on some code to check how this behave, except that looks fine.



================
Comment at: clang-tools-extra/clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp:49
                                 const LangOptions &LangOpts) {
   // FIXME: This logic breaks when there is a comment with ':'s in the middle.
+  return getRawStringRef(ReplacementRange, Sources, LangOpts).count(':') ==
----------------
check if you can do something with this...

add test for:

with something like:
```
namespace x1 {
// namespace x3::x4 {
namespace x2 {
void foo();
}
// }
}
```



================
Comment at: clang-tools-extra/clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp:70
+          File.data() + LocInfo.second, File.end());
+  L.SetCommentRetentionState(WithComment);
+  // Find the token.
----------------
HerrCai0907 wrote:
> This part of code copy from `Lexer::findNextToken` except this line. Should I refactor `Lexer::findNextToken` for example add a parameter to control it?
> But `Lexer::findNextToken` is involved with lots of code. Maybe use another patch to do it?
Dont change Lexer.

Look into clang-tools-extra/clang-tidy/utils/LexerUtils.h, if you can find something useful there,
if not then move this function there.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/modernize/Inputs/concat-nested-namespaces/modernize-concat-nested-namespaces.h:8
 // CHECK-FIXES: void t();
-// CHECK-FIXES-NEXT: } // namespace nn1
+// CHECK-FIXES-NEXT: } // namespace nn2
----------------
is check also fixes namespace comments ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147194



More information about the cfe-commits mailing list