Replace sprintf usage with snprintf #21

zach2good · 2024-11-01T12:06:10Z

Extracts one of the smaller items from this previous PR: #16

License things (I saw some mention of this in other PRs):
The files I've modified are listed under LGPL v2.1, but I'm happy to hand over all rights to the maintainers and MariaDB group, to use in whatever way they like, under whatever license they want.

lawrinn · 2024-11-03T21:04:18Z

.gitignore

@@ -39,6 +39,8 @@ benchmark/mysql-connector-cpp/
 benchmark/mariadb.json
 benchmark/mysql.json
 benchmark/main-benchmark
+cmake-build-*/


I gave some comments in the PR you referred. I'm gonna repeat them here.
These are too project-specific. I mean, I don't think they are default names of directories created by cmake/build by cmake generated projects. I am not sure I want these to be added to .gitignore for everybody. On other hand it's pretty common naming pattern. Once again - I am not sure about this

src/logger/LoggerFactory.cpp

lawrinn · 2024-11-03T23:28:55Z

src/logger/LoggerFactory.cpp

@@ -51,7 +51,7 @@ namespace mariadb
  bool LoggerFactory::initLoggersIfNeeded()
  {
    if (!NO_LOGGER) {
-      NO_LOGGER.reset(new NoLogger());
+      NO_LOGGER = Shared::Logger(new NoLogger());


In that PR I asked "Is there real need for this change? I don't think so" Now I guess the idea is to initialize in case it's not initialized. Like calling method of not initialized object is not a good idea. But it's not initialization and one line before we test that object as boolean and that involves method calling.
In 1.1 this dance around NO_LOGGER has been removed and I should do the same in 1.0 probably. But as for your PR - I guess there is probably no need to change this line

lawrinn · 2024-11-03T23:30:37Z

src/logger/LoggerFactory.h

@@ -32,9 +32,11 @@ namespace mariadb
 {
 class LoggerFactory
 {
+private:


why is that? also not needed. Everything is private in a class by default

lawrinn · 2024-11-03T23:49:23Z

Thank you for another pull request. I've made my comments.
Also please state again, that you are submitting your code under the BSD-new license. I am afraid that needs to be done for each PR

Co-authored-by: JOAQUIN BEJAR <jb@taunais.com>

zach2good · 2024-11-04T09:46:36Z

Thanks for following up and pushing that logging change into master! I've stripped down this PR so now it's just that snprintf change, and I've added the license stuff to the main post 👍

lawrinn · 2024-11-12T17:03:51Z

Out of curiosity - what do you use it for? also, since your patch against master that is 1.0 version, are you using 1.0 and if yes why not the 1.1? It is also ga, currently it is still in develop branch. To my taste it's a way better

zach2good · 2024-11-12T17:15:53Z

Ah, I assumed that `master` was whatever the running current release cycle was. I'll probably move over to using `develop` soon then! I do a lot of the engine/core work for `Final Fantasy XI` MMO server emulators. I introduced the cpp connector over our existing use of the c connector to make it easier to support prepared statements, make result access thread-safe, and be able to pull out results by column name from result sets. The change was initially born out of trying to squeeze out some more performance through prepared statements, but all the extra goodies have been great! This is the PR I added these changes in: LandSandBoat/server#4601 This is the layer I wrap the cpp connector with: https://github.com/LandSandBoat/server/blob/base/src/common/database.h#L168 I think my only issues have been having to reimplement or workaround some things that are missing in the cpp connector that I was using in the c connector, like `affectedRows`: https://github.com/LandSandBoat/server/blob/base/src/common/database.h#L225 But largely the experience has been positive, thanks for you work 👍

…

On Tue, Nov 12, 2024 at 5:04 PM lawrinn ***@***.***> wrote: Out of curiosity - what do you use it for? also, since your patch against master that is 1.0 version, are you using 1.0 and if yes why not the 1.1? It is also ga, currently it is still in develop branch. To my taste it's a way better — Reply to this email directly, view it on GitHub <#21 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAKTJIIZUMKDGLWPFJ4MNP32AIYI5AVCNFSM6AAAAABRAEYKEGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINZRGA4TSOJTHA> . You are receiving this because you authored the thread.Message ID: ***@***.***>

zach2good · 2024-11-12T17:18:53Z

Oh, I did a writeup about this upgrade process you might find interesting: https://github.com/LandSandBoat/server/wiki/Database-Library-Upgrade

…

On Tue, Nov 12, 2024 at 5:15 PM Zach Toogood ***@***.***> wrote: Ah, I assumed that `master` was whatever the running current release cycle was. I'll probably move over to using `develop` soon then! I do a lot of the engine/core work for `Final Fantasy XI` MMO server emulators. I introduced the cpp connector over our existing use of the c connector to make it easier to support prepared statements, make result access thread-safe, and be able to pull out results by column name from result sets. The change was initially born out of trying to squeeze out some more performance through prepared statements, but all the extra goodies have been great! This is the PR I added these changes in: LandSandBoat/server#4601 This is the layer I wrap the cpp connector with: https://github.com/LandSandBoat/server/blob/base/src/common/database.h#L168 I think my only issues have been having to reimplement or workaround some things that are missing in the cpp connector that I was using in the c connector, like `affectedRows`: https://github.com/LandSandBoat/server/blob/base/src/common/database.h#L225 But largely the experience has been positive, thanks for you work 👍 On Tue, Nov 12, 2024 at 5:04 PM lawrinn ***@***.***> wrote: > Out of curiosity - what do you use it for? also, since your patch against > master that is 1.0 version, are you using 1.0 and if yes why not the 1.1? > It is also ga, currently it is still in develop branch. To my taste it's a > way better > > — > Reply to this email directly, view it on GitHub > <#21 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AAKTJIIZUMKDGLWPFJ4MNP32AIYI5AVCNFSM6AAAAABRAEYKEGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINZRGA4TSOJTHA> > . > You are receiving this because you authored the thread.Message ID: > ***@***.*** > com> >

lawrinn · 2024-11-12T17:54:55Z

Well, 1.1. was supposed to become master once it goes ga, but then I've become reluctant of doing this pull requests are one of reasons - people submit their requests against master, and it's a way easier to merge from 1.0 to 1.1. then in opposite direction. Asking to rebase against different branch makes PR's a bit more cumbersome.
I didn't quite get your affected rows problem. Looking at the code it seems like you you are getting total number of rows in the resultset. But resultset has rowsCount() method. If you don't use resultset streaming it will give you the number of rows in the resultset object, otherwise it will be number of rows fetched at a time. I guess this is because of lack of documentation. In most cases you can refer to JDBC specification. The API is mostly following it
Thank you, it was interesting to read!

lawrinn · 2024-11-12T17:59:35Z

hmm, it looks like you use rowsCount(). if (rset && rset->rowsCount() && rset->next())
also, that is redundant to check here rowsCount() imho.

lawrinn reviewed Nov 3, 2024

View reviewed changes

src/logger/LoggerFactory.cpp Outdated Show resolved Hide resolved

lawrinn reviewed Nov 3, 2024

View reviewed changes

Replace usage of sprintf with snprintf

680e4bb

Co-authored-by: JOAQUIN BEJAR <jb@taunais.com>

zach2good force-pushed the various_refactors branch from 016e434 to 680e4bb Compare November 4, 2024 09:45

zach2good changed the title ~~NO_LOGGER, sprintf, and gitignore changes~~ Replace sprintf usage with snprintf Nov 4, 2024

lawrinn merged commit da8799b into mariadb-corporation:master Nov 4, 2024
0 of 2 checks passed

zach2good deleted the various_refactors branch November 12, 2024 17:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace sprintf usage with snprintf #21

Replace sprintf usage with snprintf #21

zach2good commented Nov 1, 2024 •

edited

Loading

lawrinn Nov 3, 2024

lawrinn Nov 3, 2024

lawrinn Nov 3, 2024

lawrinn commented Nov 3, 2024

zach2good commented Nov 4, 2024

lawrinn commented Nov 12, 2024

zach2good commented Nov 12, 2024 via email

zach2good commented Nov 12, 2024 via email

lawrinn commented Nov 12, 2024

lawrinn commented Nov 12, 2024

Replace sprintf usage with snprintf #21

Replace sprintf usage with snprintf #21

Conversation

zach2good commented Nov 1, 2024 • edited Loading

lawrinn Nov 3, 2024

Choose a reason for hiding this comment

lawrinn Nov 3, 2024

Choose a reason for hiding this comment

lawrinn Nov 3, 2024

Choose a reason for hiding this comment

lawrinn commented Nov 3, 2024

zach2good commented Nov 4, 2024

lawrinn commented Nov 12, 2024

zach2good commented Nov 12, 2024 via email

zach2good commented Nov 12, 2024 via email

lawrinn commented Nov 12, 2024

lawrinn commented Nov 12, 2024

zach2good commented Nov 1, 2024 •

edited

Loading