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

refactor(c/driver/postgresql): Cleanups for result_helper signatures #2261

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

WillAyd
Copy link
Contributor

@WillAyd WillAyd commented Oct 18, 2024

No description provided.

@github-actions github-actions bot added this to the ADBC Libraries 15 milestone Oct 18, 2024
@@ -89,7 +89,7 @@ class PqResultRow {
PqResultRow() : result_(nullptr), row_num_(-1) {}
PqResultRow(PGresult* result, int row_num) : result_(result), row_num_(row_num) {}

PqRecord operator[](const int& col_num) const {
PqRecord operator[](int col_num) const noexcept {
Copy link
Contributor Author

@WillAyd WillAyd Oct 18, 2024

Choose a reason for hiding this comment

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

Note the change in the paramater declaration from const int& to int

It may not matter, but I believe the const reference type just adds an unnecessary layer of indirection

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, AFAIK there is little reason to use const int&

Copy link
Member

@paleolimbot paleolimbot left a comment

Choose a reason for hiding this comment

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

I don't see any problems with this if it works, but we don't aggresively define noexcept in other places and I'm wondering why that is?

@paleolimbot
Copy link
Member

Something else to note about the PqResultHelper is that it would really better be served by being a PqQueryHelper and a PqResultHelper...the current version is an attempt to at least get all the internal querying we do in the driver in one place but there are definitely improvements (like this one!) to be had!

@WillAyd
Copy link
Contributor Author

WillAyd commented Oct 20, 2024

I don't see any problems with this if it works, but we don't aggresively define noexcept in other places and I'm wondering why that is?

Researching this some more I think there may be too much usage of noexcept in the current PR implementation. Here are guidelines I found from the stdlib:

https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2011/n3279.pdf

My take on it is that many of these methods are "narrow" (i.e. they have some precondition / state) so probably not worth marking this way.

@lidavidm
Copy link
Member

noexcept is tricky to use. I think ReleaseResult is the only thing here that may be marked noexcept, and some of the very trivial getters/setters. Any memory allocation may throw and an assignment may actually be a copy so it often does not apply even if the body seems trivial.

@WillAyd
Copy link
Contributor Author

WillAyd commented Oct 21, 2024

OK cool - I'll scale the noexcept back a lot here.

Any memory allocation may throw

FWIW the cpp core guidelines suggest that you may still want to mark functions that can throw because of OOM issues as noexcept:

https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#f6-if-your-function-must-not-throw-declare-it-noexcept

@lidavidm
Copy link
Member

Ah, good to know. I suppose that means on the contrary nearly everything here could be noexcept, since we don't use exceptions...but in that case we may as well disable exceptions altogether if we're that sure

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.

3 participants