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

Binding BLOB requires narrowing conversion #430

Open
fzakaria opened this issue Jun 10, 2023 · 5 comments · May be fixed by #431
Open

Binding BLOB requires narrowing conversion #430

fzakaria opened this issue Jun 10, 2023 · 5 comments · May be fixed by #431
Assignees

Comments

@fzakaria
Copy link

const int lengthInBytes =
      static_cast<int>(sizeof(uint8_t) * binary->raw().size());
insert.bind(1, binary->raw().data(), lengthInBytes);

Would be nice if the API took size_t or something so that I don't have to narrow it.
On my machine size_type (size_t) is unsigned long.

@SRombauts SRombauts self-assigned this Jun 12, 2023
@SRombauts
Copy link
Owner

Interestingly, I am pretty sure that this was handled in the past, where I made it support short/int/long/long long etc old types from C, but it was refactored to now rely on more modern types ala uint_t.

It would make sense to perfectly support this use case and the STL in general, so I'll take a look at the current state of affairs.

What is the system you are working on?

@SRombauts
Copy link
Owner

For the record, I wasn't able to get a warning or a compilation error with the unit tests there are using size_t to bind a blob (tested with VS on Windows and clang on WSL)

@fzakaria
Copy link
Author

Looks like it's from clang-tidy, specifically bugprone-narrowing-conversions

rc/cli/elf2sql.cc:41:40: warning: narrowing conversion from 'std::vector<unsigned char>::size_type' (aka 'unsigned long') to signed type 'int' is implementation-defined [bugprone-narrowing-conversions]
  insert.bind(1, binary->raw().data(), binary->raw().size());
                                       ^

Sounds like a real issue and that size_t should be the type that is accepted?

SRombauts added a commit that referenced this issue Jun 12, 2023
Fix #430 for a better support of standard containers

Also add a couple of tests (not covering all variants)
@SRombauts
Copy link
Owner

SRombauts commented Jun 12, 2023

It's not a "real-life issue" since SQLiteCpp can only bind a blob with a size in int.
I created a PR nonetheless, but I'll take the time to think about it twice; it might be better in fact to NOT try to hide that and let the developer be aware of the limitations of the underlying API

@tamaskenez
Copy link

@SRombauts There's no such limitation, sqlite3_bind_blob64 takes sqlite3_uint64 as size.

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

Successfully merging a pull request may close this issue.

3 participants