Skip to content

Conversation

@wiluite
Copy link

@wiluite wiluite commented Oct 1, 2025

Hi.
This PR is about more concise code for the purpose of its clear understanding by those interested.

I'm just learning template metaprogramming from your projects.
I noticed that when forming a query within the params_format_builder, format_selector, and nth_param template classes, numeric indexing isn't actually used, and apparently doesn't matter, since the data types themselves are sufficient to generate all the necessary data for output.
And perhaps the numeric indexing was left in the code for the convenience of quick debugging at the right moment.
If it does make sense in the release version, could you please clarify?

Otherwise, you could decide to accept this.
Thanks!

@zmij
Copy link
Owner

zmij commented Oct 6, 2025

Thank you so much for taking the time to submit this PR and for the kind words about the library! I really appreciate you diving into the code and identifying these improvements.
I have to admit, it's been about 7 years since I last worked on this project, so I don't recall the intricate details of why those particular type index choices were made originally. Your analysis looks thorough and well-reasoned.
You're absolutely right that in compiled code, these type index optimisations don't affect runtime performance - the compiler sorts it all out regardless. The improvements you've made seem to be more about code clarity and following better practices, which is always valuable.
Have you had a chance to run the tests to make sure everything still builds and works correctly? I never set up proper CI for this library (something I probably should have done!), so manual testing is still the way to go.
I'm grateful that people like you are still finding the library useful and taking the time to contribute back. Thank you for keeping the project alive with your contribution!

@wiluite
Copy link
Author

wiluite commented Oct 9, 2025

Yes, all tests of the project are still working/passed, including QueryParamsWriteTest in module db_io_tests.cpp.

I noticed that param format builder specialization for params that are only in text format seems not engaged in QueryParamsWriteTest (this specialization seems picked up for some other tests but not for that). If need be, I probably could add yet another test (say, QueryTextParamsWriteTest) to db_io_test.cpp to check against say: ...make_test_data(std::string("a"), std::string("b")...).

@zmij
Copy link
Owner

zmij commented Oct 11, 2025

There's no need to test text parsing extensively, the hard part is binary protocol. I'll merge the pr as soon as the power outage is over.

Just curious, are you using the library in some project or just for studying purposes?

For production purposes I'd recommend you the https://userver.tech C++ framework, it has a postgres driver also written by me. I It's binary protocol only and template magic is more elegant there

@wiluite
Copy link
Author

wiluite commented Oct 18, 2025

Yes, just for studying, for learning techniques, approaches, and so on in high-professional code.

Im just an old programmer (with young soul;-), retired from previous occupation (not IT) and having time to look around. Such things like metapushkin/afsm/pg_async were, for years, unclosed gestalt for me (especially afsm), and this year sometime I worked hard to close it in key aspects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants