https://github.com/unterumarmung requested changes to this pull request.
Thanks for working on this, and welcome to LLVM! I’m going to mark this version as “request changes”, but please don’t read that as a big deal. The idea is good; the current patch just needs to meet the usual clang-tidy bar before it can land. Suggested PR title: `[clang-tidy] Extend readability-redundant-parentheses to declarations` Suggested description: ```text Extend readability-redundant-parentheses to diagnose redundant parentheses in declarations, for example: int (x); -> int x; void (f)(int (arg)); -> void f(int arg); This keeps the existing expression diagnostics and adds declaration-specific coverage, tests, documentation, and release notes. ``` If it fixes a particular issue from the bug-tracker, also append like that says something like this: "Fixes #<issue-num>", so it auto-closes on the merge. There are a few mechanical issues first. The formatter and linter are already reporting problems. Please run `git-clang-format` and the clang-tidy diff command from the bot comment before updating. The implementation also needs more care. `typeLoc(loc(parenType()))` is very broad, and declaration parentheses are not always redundant. For example, these parentheses are required and removing them changes the declaration: ```cpp int (*fp)(int); int (S::*mp); int (*array[2])(int); ``` Please add negative tests for cases like these and make sure the matcher does not diagnose them. If the intended scope is only cases like `int (x);` and parenthesized parameter names, the matcher should be narrowed to exactly those forms, with the fix-it only emitted when removing the tokens is known to preserve the type and parse correctly. It would also be good to cover macros and templates, since clang-tidy checks often regress there if the matcher is too broad. This patch also needs tests. Please extend `clang-tools-extra/test/clang-tidy/checkers/readability/redundant-parentheses.cpp` with positive and negative cases, including fix-it checks. Examples to cover: ```cpp int (x); // CHECK-MESSAGES: warning: redundant parentheses in declaration // CHECK-FIXES: int x; void f(int (arg)); // CHECK-MESSAGES: warning: redundant parentheses in declaration // CHECK-FIXES: void f(int arg); int (*fp)(int); // no warning int (&ref); // no warning if removing would change meaning or fail ``` Please also add cases for function declarations, function pointer declarations, parameters, templates, and macro spelling/expansion if this check is expected to handle them. Since this changes user-visible behavior, please update both the check documentation and release notes. The documentation at `clang-tools-extra/docs/clang-tidy/checks/readability/redundant-parentheses.rst` should mention declarations and show before/after examples. `clang-tools-extra/docs/ReleaseNotes.rst` should get an entry for the new behavior. Open question for maintainers: should declaration parentheses be part of `readability-redundant-parentheses`, or should this be a separate check? If it stays here, should the check grow options such as `WarnOnExpressions` and `WarnOnDeclarations` so users can enable the two behaviors independently? Finally, please take a look at the LLVM coding standards and clang-tidy contributing guide before the next revision: - https://llvm.org/docs/CodingStandards.html - https://clang.llvm.org/extra/clang-tidy/Contributing.html https://github.com/llvm/llvm-project/pull/196739 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
