Page MenuHomePhabricator

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

Authored by HerrCai0907 on Mar 29 2023, 7:09 PM.



Partially fixed #60035
This patch refactor the FixHint for concat-nest-namespace.

  1. remove each namespace except the last non-nest namespace.
  2. replace the last non-nest namespace with the new name.

It can remain the comment / pragma / macro between namespace and update the close comment.

Diff Detail

Event Timeline

HerrCai0907 created this revision.Mar 29 2023, 7:09 PM
HerrCai0907 requested review of this revision.Mar 29 2023, 7:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 29 2023, 7:09 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
HerrCai0907 added inline comments.Mar 30 2023, 8:56 AM

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?

You changed half of this check logic, please take a look into those issues for this check:
Add issues that you fixing to commit message.

Also look into these patches:

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.


check if you can do something with this...

add test for:

with something like:

namespace x1 {
// namespace x3::x4 {
namespace x2 {
void foo();
// }

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.


is check also fixes namespace comments ?

PiotrZSL added inline comments.Mar 30 2023, 12:31 PM

ok, neverming... lets let other check fix them...

HerrCai0907 marked 3 inline comments as done.Mar 30 2023, 4:51 PM
HerrCai0907 added inline comments.

It still have false negative.
But this should be done in another PR. In here I just extract a common function without changing the logic.

HerrCai0907 marked an inline comment as done.

refactor some code and add release note

HerrCai0907 edited the summary of this revision. (Show Details)Apr 1 2023, 6:15 PM
PiotrZSL added inline comments.Apr 2 2023, 9:03 AM

Remove this debug.


what if it is //namespace ? or /* namespace XYZ */


remove redundant {} from this function, they not needed for single line statements.


add same test but without spaces and move //CHECK-MESSAGES outside namespace.

HerrCai0907 edited the summary of this revision. (Show Details)

add feature update close comment in fixhint
update acc. comment

HerrCai0907 edited the summary of this revision. (Show Details)Apr 3 2023, 6:17 PM

Overall looks +- fine, these are last comments from me.


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

this index looks reundant, you could use range-for


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

update according to comment

HerrCai0907 marked 4 inline comments as done.

reorder fixhint

HerrCai0907 marked 2 inline comments as done.Apr 7 2023, 12:09 PM
HerrCai0907 added inline comments.

I think it can cover most of scenarios. It don't worth to introduce a mechanism to archive it.
Because something false positive like // name space XYZ should also be considered.

PiotrZSL accepted this revision.Apr 7 2023, 12:24 PM
This revision is now accepted and ready to land.Apr 7 2023, 12:24 PM