Pre-define atoms #299
Pre-define atoms #299
Conversation
| static ERL_NIF_TERM am_rows; | ||
| static ERL_NIF_TERM am_invalid_filename; | ||
| static ERL_NIF_TERM am_invalid_flags; | ||
| static ERL_NIF_TERM am_database_open_failed; |
There was a problem hiding this comment.
This should be rc + exception, since otherwise it's hard to understand why the open failed. Could be file permissions, could be something else. I think I actually had a problem with this one in the past.
There was a problem hiding this comment.
Oh I saw this being done in another nif implementation and didn't understand the optimization being done when I initially implemented this. So I just created the atoms on demand rather than all up front at once.
| static ERL_NIF_TERM am_connection_closed; | ||
| static ERL_NIF_TERM am_invalid_statement; | ||
| static ERL_NIF_TERM am_invalid_chunk_size; | ||
| static ERL_NIF_TERM am_busy; |
There was a problem hiding this comment.
This should be rc + exception, since the error message might contain useful info.
| static ERL_NIF_TERM am_idle; | ||
| static ERL_NIF_TERM am_database_name_not_iolist; | ||
| static ERL_NIF_TERM am_serialization_failed; | ||
| static ERL_NIF_TERM am_deserialization_failed; |
There was a problem hiding this comment.
I haven't used this API before, but I think it might also benefit from being rc + exception.
c_src/sqlite3_nif.c
Outdated
| @@ -447,197 +437,6 @@ exqlite_prepare(ErlNifEnv* env, int argc, const ERL_NIF_TERM argv[]) | |||
| return make_ok_tuple(env, result); | |||
| } | |||
|
|
|||
| static ERL_NIF_TERM | |||
| bind(ErlNifEnv* env, const ERL_NIF_TERM arg, sqlite3_stmt* statement, int index) | |||
There was a problem hiding this comment.
Did you mean to remove bind in this PR?
There was a problem hiding this comment.
Yeah, I kind of planned that #298 would get merged and then I would continue this PR but then got distracted :)
I'll finish it tomorrow morning.
There was a problem hiding this comment.
That sounds good. Thanks for the help.
There was a problem hiding this comment.
I think this PR is done, and I'll open another one (#301) to explore replacing some of the current atoms with raise_badarg() helper.
| @@ -594,7 +585,7 @@ make_cell(ErlNifEnv* env, sqlite3_stmt* statement, unsigned int i) | |||
| sqlite3_column_bytes(statement, i)); | |||
|
|
|||
| default: | |||
| return make_atom(env, "unsupported"); | |||
| return am_nil; | |||
There was a problem hiding this comment.
This technically doesn't matter since there are only five types in SQLite, all covered by the cases above. If anything, this should probably raise an exception. Retuning nil or :unsupported seems a bit arbitrary.
There was a problem hiding this comment.
I don't think it is possible to reach this state, I defined it just in case it did happen. I don't know if extensions can even set this to a custom type.
| } | ||
|
|
||
| static ERL_NIF_TERM | ||
| make_bind_error(ErlNifEnv* env, ERL_NIF_TERM message, ERL_NIF_TERM argument) |
There was a problem hiding this comment.
I forgot to remove this function in the individual binds PR. So I'm removing it in this PR.
f4ed9ad to
5ca194f
Compare
This PR predefines atoms like Rustler does. I think this improves readability and kind of makes it safer against typos. Once or if #298 is merged, many of the currently used atoms could be replaced with
raise_badarg()function. I'll also leave some notes on atoms that I think shouldn't be atoms and rather SQLite errors.