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

increase warning level on C/C++ compiler #1689

Merged
merged 7 commits into from
Mar 12, 2024
Merged

increase warning level on C/C++ compiler #1689

merged 7 commits into from
Mar 12, 2024

Conversation

arogge
Copy link
Member

@arogge arogge commented Jan 29, 2024 •
edited by sebsura

This PR adds new warnings and refactors code where needed

  • enable -Wshadow and refactor problematic code
  • enable -Wnon-virtual-dtor

Please check

  • Short description and the purpose of this PR is present above this paragraph
  • Your name is present in the AUTHORS file (optional)

If you have any questions or problems, please give a comment in the PR.

Helpful documentation and best practices

Checklist for the reviewer of the PR (will be processed by the Bareos team)

Make sure you check/merge the PR using devtools/pr-tool to have some simple automated checks run and a proper changelog record added.

General
  • Is the PR title usable as CHANGELOG entry?
  • Purpose of the PR is understood
  • Commit descriptions are understandable and well formatted
    Check backport line
    Required backport PRs have been created
Source code quality
  • Source code changes are understandable
  • Variable and function names are meaningful
  • Code comments are correct (logically and spelling)
  • Required documentation changes are present and part of the PR

@arogge arogge force-pushed the dev/arogge/master/spring-cleanup branch 3 times, most recently from 7767d56 to d3f4e9c Compare January 31, 2024 11:32
@arogge arogge force-pushed the dev/arogge/master/spring-cleanup branch from 481c0ba to 78bf5df Compare February 6, 2024 12:51
@arogge arogge force-pushed the dev/arogge/master/spring-cleanup branch 2 times, most recently from 5def8a2 to b2e994b Compare February 15, 2024 16:06
@arogge arogge changed the title Spring Cleanup 2024 increase warning level on C/C++ compiler Feb 15, 2024
@arogge arogge marked this pull request as ready for review February 15, 2024 16:07
@sebsura sebsura self-requested a review February 16, 2024 08:18
sebsura
sebsura requested changes Feb 16, 2024
View reviewed changes
Copy link
Contributor

@sebsura sebsura left a comment

Choose a reason for hiding this comment

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

I like the new warnings, but in general i feel like we should not rename local variables of functions but instead make it clearer that a global variable is global by renaming the global variable.

core/src/dird/ua_output.cc Outdated Show resolved Hide resolved
core/src/cats/cats.h Outdated Show resolved Hide resolved
core/src/cats/sql_get.cc Outdated Show resolved Hide resolved
core/src/cats/sql_get.cc Outdated Show resolved Hide resolved
core/src/cats/sql_get.cc Outdated Show resolved Hide resolved
core/src/stored/backends/win32_tape_device.cc Show resolved Hide resolved
core/src/stored/bcopy.cc Outdated Show resolved Hide resolved
core/src/stored/bextract.cc Outdated Show resolved Hide resolved
core/src/stored/btape.cc Show resolved Hide resolved
core/src/stored/btape.cc Outdated Show resolved Hide resolved
@arogge arogge requested a review from sebsura February 29, 2024 21:35
@arogge
Copy link
Member Author

arogge commented Feb 29, 2024

Please take another look. I have addressed most of your requested changes and left comments on the others.

sebsura
sebsura requested changes Mar 4, 2024
View reviewed changes
Copy link
Contributor

@sebsura sebsura left a comment

Choose a reason for hiding this comment

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

Thanks for the changes. I added some comments as well as adding some comments to the previous (open) conversations as well.

core/src/dird/ua_output.cc Outdated Show resolved Hide resolved
core/src/dird/ua_output.cc Outdated Show resolved Hide resolved
core/src/dird/ua_output.cc Outdated Show resolved Hide resolved
core/src/dird/ua_output.cc Outdated Show resolved Hide resolved
core/src/cats/cats.h Outdated Show resolved Hide resolved
core/src/dird/dbcheck.cc Outdated Show resolved Hide resolved
core/src/lib/bareos_resource.h Outdated Show resolved Hide resolved
core/src/lib/bsock_tcp.cc Outdated Show resolved Hide resolved
core/src/lib/messages_resource.h Outdated Show resolved Hide resolved
sebsura
sebsura approved these changes Mar 8, 2024
View reviewed changes
Copy link
Contributor

@sebsura sebsura left a comment

Choose a reason for hiding this comment

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

Thanks for the work! Looks good to me!

@arogge arogge force-pushed the dev/arogge/master/spring-cleanup branch from 248003e to 290eca5 Compare March 8, 2024 08:09
@sebsura sebsura force-pushed the dev/arogge/master/spring-cleanup branch from 290eca5 to a5b9c64 Compare March 8, 2024 11:09
arogge and others added 7 commits March 12, 2024 15:41
@arogge @BareosBot
refactor: add -Wshadow build option
812f9e4 
and refactor all code that triggers that warning.
@arogge @BareosBot
cmake: add -Wnon-virtual-dtor C++ build option
98ccaa1
@arogge @BareosBot
dbcheck: remove messages guarded by g_verbose > 1
ca40217 
As there is no way to set g_verbose to anything but 0 or 1 in dbcheck,
this code is unrechable and can be removed safely.
@arogge @BareosBot
dird: add new function GetArgValue()
c9cacf4 
this function can simplify the current use of FindArgWithValue(). It
does the same thing, but instead of returning the index, it returns a
pointer to the value, or nullptr if the argument was not found or did
not have a value.
@arogge @BareosBot
dird: use GetArgValue() in ua_output.cc where feasible
f4caed7
@sebsura @BareosBot
wrap: fix shadowing key
bc65572
@BareosBot
Update CHANGELOG.md
ab10dbd
@BareosBot BareosBot force-pushed the dev/arogge/master/spring-cleanup branch from 5579484 to ab10dbd Compare March 12, 2024 15:41
@BareosBot BareosBot merged commit 21a1a23 into bareos:master Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sebsura sebsura sebsura approved these changes

Assignees

@sebsura sebsura

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@arogge @sebsura @BareosBot