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.

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

Resolve clang-tidy warnings for Xapian wrapper #5017

Merged
merged 10 commits into from
Sep 18, 2024

Conversation

rsto
Copy link
Member

@rsto rsto commented Sep 4, 2024

The current Xapian wrapper code in imap/xapian_wrap.cpp is anything but idiomatic or modern C++ code. #2818 highlights some of that, but we didn't act on it timely and the code now is stale. It also didn't help that the code changes are all in a single commit.

This PR attempts to sanitize the C++ code in smaller steps. As a baseline of what constitutes "sane C++" code, we'll start with the clang-tidy linter with the bugprone check category.

I am using this with Debian LLVM version 14.0.6.

@rsto rsto marked this pull request as draft September 4, 2024 10:35
@rsto
Copy link
Member Author

rsto commented Sep 4, 2024

Once we are done with the clang-tidy changes, let's get back to #2818 and see if there is anything in there that's not addressed already.

@rsto rsto self-assigned this Sep 4, 2024
imap/xapian_wrap.cpp Show resolved Hide resolved
imap/xapian_wrap.cpp Outdated Show resolved Hide resolved
@rsto rsto force-pushed the clang_tidy_xapian_wrap branch 5 times, most recently from 2c932df to 5d53b57 Compare September 12, 2024 06:26
@rsto rsto marked this pull request as ready for review September 12, 2024 06:59
@rsto
Copy link
Member Author

rsto commented Sep 12, 2024

I have updated this PR description, this PR only is meant to fix issues found by the bugprone clang-tidy category. I will submit separate PRs for further categories.

Copy link
Contributor

@ksmurchison ksmurchison left a comment

Choose a reason for hiding this comment

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

If it compiles and runs...

Copy link
Contributor

@elliefm elliefm 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 these changes, couple of nits though

imap/xapian_wrap.cpp Outdated Show resolved Hide resolved
imap/xapian_wrap.cpp Outdated Show resolved Hide resolved
@elliefm elliefm self-requested a review September 16, 2024 00:06
@rsto rsto merged commit c182ce6 into cyrusimap:master Sep 18, 2024
1 check passed
@rsto rsto deleted the clang_tidy_xapian_wrap branch September 18, 2024 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants