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

Update logging to use C++ strings, try to get rid of log.h #1182

Closed
uweseimet opened this issue Jul 4, 2023 · 3 comments · Fixed by #1232
Closed

Update logging to use C++ strings, try to get rid of log.h #1182

uweseimet opened this issue Jul 4, 2023 · 3 comments · Fixed by #1232
Assignees
Labels
code quality enhancement New feature or request

Comments

@uweseimet
Copy link
Contributor

uweseimet commented Jul 4, 2023

This should be addressed after the bookworm related changes have been committed in order to avoid merge conflicts (see #1179).

The spdlog logging methods (spdlog::info, spdlog::debug etc.) accept C++ strings/string_views as arguments. This means that instead of

LOGINFO("Renamed/Moved image file '%s' to '%s'", from.c_str(), to.c_str())

just like with any other library we can just write

spdlog::info("Renamed/Moved image file '" + from + "' to '" + to + "'");

There is no need for using macros, format strings, varargs or a static buffer for log messages, and log.h most likely becomes obsolete. Note that current versions of C++ in general aim at reducing the need for using macros.

In addition, a convenience function like

string Strerrno(const string& msg) {
	return msg + ": " + string(strerror(errno));
}

shall be introduced.

@uweseimet uweseimet self-assigned this Jul 4, 2023
@uweseimet uweseimet added the enhancement New feature or request label Sep 22, 2023
@uweseimet
Copy link
Contributor Author

uweseimet commented Sep 25, 2023

@rdmark Just in case you (or somebody else, of course) can spare some time: If everything goes to plan (also see #1213) I will create a PR for #1179 and then #1182 next week. #1182 includes all changes from #1179. I have cleaned up a lot of code (except for the legacy stuff), and by making better use of C++ features (not only C++-20 but also older) rather tricky SonarQube issues have been resolved and the code volume has decreased without losing functionality. Based on my changes I could also improve the test coverage and reduce the object dependencies. There are also some minor bug fixes, which would not have justified a separate ticket.
I would welcome help with testing #1182 (not #1179, it's just an intermediate step). The respective branch is test_issue_1182. There are no known issues so far (I tested with bullseye and bookworm), but Murphy's law says that somebody else will find some ;-).

@rdmark
Copy link
Member

rdmark commented Sep 25, 2023

@uweseimet Thanks for the heads up! This week I'm in the middle of preparing a move from Seattle to Tokyo (for work) so I won't have time for piscsi for some time, unfortunately. Once I'm settled down, maybe 2-3 months later, I'll lean back into the project and see what I can do to help.

@uweseimet
Copy link
Contributor Author

@rdmark Seattle to Tokyo, wow! I hope everything works as planned.

uweseimet added a commit that referenced this issue Oct 1, 2023
@uweseimet uweseimet linked a pull request Oct 1, 2023 that will close this issue
uweseimet added a commit that referenced this issue Oct 2, 2023
@uweseimet uweseimet linked a pull request Oct 2, 2023 that will close this issue
@uweseimet uweseimet linked a pull request Oct 2, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code quality enhancement New feature or request
Projects
None yet
2 participants