Skip to content

enabled and mitigated readability-const-return-type clang-tidy warnings#5644

Merged
chrchr-github merged 1 commit intocppcheck-opensource:mainfrom
firewave:tidy-const-return
Nov 24, 2023
Merged

enabled and mitigated readability-const-return-type clang-tidy warnings#5644
chrchr-github merged 1 commit intocppcheck-opensource:mainfrom
firewave:tidy-const-return

Conversation

@firewave
Copy link
Copy Markdown
Collaborator

@firewave firewave commented Nov 9, 2023

No description provided.

@firewave firewave marked this pull request as draft November 9, 2023 10:39
@firewave
Copy link
Copy Markdown
Collaborator Author

firewave commented Nov 9, 2023

Still need to file upstream tickets about the false positive and lacking context:

/home/runner/work/cppcheck/cppcheck/lib/utils.h:39:5: error: return type 'typename pair<const int, const Variable *>::first_type' (aka 'const int') is 'const'-qualified at the top level, which may reduce code readability without improving const correctness [readability-const-return-type,-warnings-as-errors]
   39 |     typename Pair::first_type operator()(const Pair& p) const {
      |     ^

@firewave firewave marked this pull request as ready for review November 9, 2023 11:15
chrchr-github
chrchr-github reviewed Nov 9, 2023
View reviewed changes
Comment thread gui/projectfiledialog.cpp Outdated
void ProjectFileDialog::setSuppressions(const QList<Suppressions::Suppression> &suppressions)
{
mUI->mListSuppressions->clear();
// NOLINTNEXTLINE(readability-const-return-type) - neeeds to be a copy since it might be a reference to mSuppressions
Copy link
Copy Markdown
Collaborator

@chrchr-github chrchr-github Nov 9, 2023

Choose a reason for hiding this comment

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

Is this really related?

Copy link
Copy Markdown
Collaborator Author

@firewave firewave Nov 9, 2023

Choose a reason for hiding this comment

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

What do you mean?

Copy link
Copy Markdown
Collaborator

@chrchr-github chrchr-github Nov 9, 2023

Choose a reason for hiding this comment

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

There is no return type on that line, so what does clang-tidy warn about?

Copy link
Copy Markdown
Collaborator Author

@firewave firewave Nov 9, 2023

Choose a reason for hiding this comment

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

Good point. Maybe yet another false positive in this check. Although it is a valid warning and possibly even a false negative on our side.

Copy link
Copy Markdown
Collaborator

@chrchr-github chrchr-github Nov 9, 2023

Choose a reason for hiding this comment

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

Sounds like the NOLINT should be for performance-unnecessary-copy-initialization?

Copy link
Copy Markdown
Collaborator Author

@firewave firewave Nov 9, 2023

Choose a reason for hiding this comment

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

If it would report that warning and we would not suppress it correctly the CI would fail.

Copy link
Copy Markdown
Collaborator

@chrchr-github chrchr-github Nov 9, 2023

Choose a reason for hiding this comment

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

Yeah, but unmatched NOLINTs don't seem to cause a warning: llvm/llvm-project#69674

Copy link
Copy Markdown
Collaborator Author

@firewave firewave Nov 18, 2023

Choose a reason for hiding this comment

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

Yeah - this is unrelated. Will revert.

Copy link
Copy Markdown
Collaborator Author

@firewave firewave Nov 24, 2023

Choose a reason for hiding this comment

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

Done.

@firewave firewave marked this pull request as draft November 9, 2023 12:14
@firewave
Copy link
Copy Markdown
Collaborator Author

firewave commented Nov 24, 2023

I filed llvm/llvm-project#73270 about the missing context.

@firewave firewave marked this pull request as ready for review November 24, 2023 00:27
@chrchr-github chrchr-github merged commit 86bb7c9 into cppcheck-opensource:main Nov 24, 2023
@firewave firewave deleted the tidy-const-return branch November 24, 2023 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chrchr-github chrchr-github chrchr-github left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@firewave @chrchr-github