Skip to content

[clang-format] Don't swap (const override) with QAS_Right #167191

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+16 −5
Open

[clang-format] Don't swap (const override) with QAS_Right #167191

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 14 additions & 5 deletions clang/lib/Format/QualifierAlignmentFixer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,13 +177,16 @@ static bool isQualifier(const FormatToken *const Tok) {

const FormatToken *LeftRightQualifierAlignmentFixer::analyzeRight(
const SourceManager &SourceMgr, const AdditionalKeywords &Keywords,
tooling::Replacements &Fixes, const FormatToken *const Tok,
tooling::Replacements &Fixes, const FormatToken *Tok,
Copy link
Contributor

@HazardyKnusperkeks HazardyKnusperkeks Nov 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd keep that. We want most of our locals to be const, why not arguments?

Copy link
Contributor Author

@owenca owenca Nov 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's superfluous. It's similar to the const in void f(const int);.

Copy link
Contributor

@HazardyKnusperkeks HazardyKnusperkeks Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No it's similar to void foo(const int i) {...} and as such has the same prevalence as the const in const auto E = Changes.size(), which we regularly request.

Copy link
Contributor Author

@owenca owenca Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My point was that adding const to a parameter passed by value was redundant. Are you really saying that we should add const to all such parameters? Then you would do the same to tok::TokenKind QualifierType one line below?

Actually, this is the only file in clang/lib/Format/ that has *const before parameters, and we should remove all of them. See https://stackoverflow.com/a/60823004.

Copy link
Contributor

@HazardyKnusperkeks HazardyKnusperkeks Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually do that, yes. As said, it's the same as local variables. But you have the approval, you can merge this.

const std::string &Qualifier, tok::TokenKind QualifierType) {
// We only need to think about streams that begin with a qualifier.
if (Tok->isNot(QualifierType))
return Tok;

const auto *Next = Tok->getNextNonComment();

// Don't concern yourself if nothing follows the qualifier.
if (!Tok->Next)
if (!Next)
return Tok;

// Skip qualifiers to the left to find what preceeds the qualifiers.
Expand Down Expand Up @@ -247,9 +250,15 @@ const FormatToken *LeftRightQualifierAlignmentFixer::analyzeRight(
}();

// Find the last qualifier to the right.
const FormatToken *LastQual = Tok;
while (isQualifier(LastQual->getNextNonComment()))
LastQual = LastQual->getNextNonComment();
const auto *LastQual = Tok;
for (; isQualifier(Next); Next = Next->getNextNonComment())
LastQual = Next;

if (!LastQual || !Next ||
(LastQual->isOneOf(tok::kw_const, tok::kw_volatile) &&
Next->isOneOf(Keywords.kw_override, Keywords.kw_final))) {
return Tok;
}

// If this qualifier is to the right of a type or pointer do a partial sort
// and return.
Expand Down
2 changes: 2 additions & 0 deletions clang/unittests/Format/QualifierFixerTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,8 @@ TEST_F(QualifierFixerTest, RightQualifier) {
Style);
verifyFormat("void foo() const override;", Style);
verifyFormat("void foo() const override LLVM_READONLY;", Style);
verifyFormat("MOCK_METHOD(ReturnType, myMethod, (int), (const override));",
Style);
verifyFormat("void foo() const final;", Style);
verifyFormat("void foo() const final LLVM_READONLY;", Style);
verifyFormat("void foo() const LLVM_READONLY;", Style);
Expand Down