Skip to content
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

Jump to bottom

cleanup: Fix a few more clang-tidy warnings. #2405

Merged
merged 1 commit into from
Sep 7, 2023
Merged

cleanup: Fix a few more clang-tidy warnings. #2405

merged 1 commit into from
Sep 7, 2023

Conversation

iphydf
Copy link
Member

@iphydf iphydf commented Aug 31, 2023 •
edited
Loading

This change is Reviewable

@iphydf iphydf added this to the v0.2.19 milestone Aug 31, 2023
@iphydf iphydf changed the title Init vars cleanup: Initialise all local variables. Aug 31, 2023
@iphydf iphydf force-pushed the init-vars branch 3 times, most recently from ad1930c to 608c881 Compare September 1, 2023 22:16
@iphydf iphydf marked this pull request as ready for review September 1, 2023 22:16
@iphydf iphydf force-pushed the init-vars branch 6 times, most recently from 36c3a24 to 722098a Compare September 1, 2023 22:52
nurupo
nurupo requested changes Sep 2, 2023
View reviewed changes
other/bootstrap_daemon/src/config.c Outdated Show resolved Hide resolved
other/bootstrap_daemon/src/config.c Outdated Show resolved Hide resolved
other/bootstrap_daemon/src/config.c Outdated Show resolved Hide resolved
other/bootstrap_daemon/src/config.c Outdated Show resolved Hide resolved
other/bootstrap_daemon/src/config.c Outdated Show resolved Hide resolved
other/bootstrap_daemon/src/config.c Outdated Show resolved Hide resolved
other/bootstrap_daemon/src/tox-bootstrapd.c Outdated Show resolved Hide resolved
other/bootstrap_daemon/src/tox-bootstrapd.c Outdated Show resolved Hide resolved
@nurupo
Copy link
Member

nurupo commented Sep 2, 2023

It's unnecessary in any of these cases, but having the warning is
useful, and may catch errors in the future.

Which warning? Variable assigned but never used?

@nurupo
Copy link
Member

nurupo commented Sep 2, 2023

Speaking of warnings, is there a way to make a custom token warning, so that we would get warned if someone uses NULL instead of nullptr?

@iphydf
Copy link
Member Author

iphydf commented Sep 2, 2023

Speaking of warnings, is there a way to make a custom token warning, so that we would get warned if someone uses NULL instead of nullptr?

We have that, yes. In one of the builds we get an error for it.

@iphydf
Copy link
Member Author

iphydf commented Sep 2, 2023

It's unnecessary in any of these cases, but having the warning is
useful, and may catch errors in the future.

Which warning? Variable assigned but never used?

"Variable not initialised"

@pull-request-attention pull-request-attention bot assigned nurupo and unassigned iphydf Sep 2, 2023
@iphydf iphydf changed the title cleanup: Initialise all local variables. cleanup: Fix a few more clang-tidy warnings. Sep 2, 2023
@codecov
Copy link

codecov bot commented Sep 2, 2023 •
edited
Loading

Codecov Report

Patch coverage has no change and project coverage change: -0.09% ⚠️

Comparison is base (4d3c97f) 74.57% compared to head (0c5b918) 74.49%.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #2405      +/-   ##
==========================================
- Coverage   74.57%   74.49%   -0.09%     
==========================================
  Files          87       87              
  Lines       26194    26194              
==========================================
- Hits        19533    19512      -21     
- Misses       6661     6682      +21

see 7 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@iphydf
Copy link
Member Author

iphydf commented Sep 3, 2023

I've changed it back. This change conflicts too much with other warnings that are more useful.

JFreegman
JFreegman approved these changes Sep 3, 2023
View reviewed changes
Copy link
Member

@JFreegman JFreegman left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 87 files at r1, 84 of 84 files at r2, all commit messages.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @nurupo)

@iphydf
Copy link
Member Author

iphydf commented Sep 4, 2023

@nurupo PTAL. I've addressed your comments, I think (and also reverted most of it).

@nurupo
Copy link
Member

nurupo commented Sep 4, 2023 •
edited
Loading

Ah, sorry for the wait, code review replies and @ mentions don't stand out from regular TokTok email spam in my mailbox so I almost missed this, opened it by a pure chance just now.

nurupo
nurupo approved these changes Sep 4, 2023
View reviewed changes
Copy link
Member

@nurupo nurupo left a comment

Choose a reason for hiding this comment

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

LGTM

@nurupo
Copy link
Member

nurupo commented Sep 4, 2023

code review replies and @ mentions don't stand out from regular TokTok email spam in my mailbox

Actually that's false, they do stand out, was just looking at the wrong gmail label when writing this.

@iphydf iphydf merged commit 0c5b918 into TokTok:master Sep 7, 2023
49 of 52 checks passed
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JFreegman JFreegman JFreegman approved these changes

@nurupo nurupo nurupo approved these changes

@Green-Sky Green-Sky Green-Sky approved these changes

Assignees

@iphydf iphydf

Labels
None yet
Projects
None yet
Milestone
v0.2.19
Development

Successfully merging this pull request may close these issues.
4 participants
@iphydf @nurupo @JFreegman @Green-Sky