From: Kirill Shcherbatov <kshcherbatov@tarantool.org> To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>, Tarantool MailList <tarantool-patches@freelists.org> Subject: [tarantool-patches] Re: [PATCH v1 3/3] box: extend ffi error object API Date: Tue, 6 Aug 2019 10:56:46 +0300 [thread overview] Message-ID: <c9b389bf-b1f5-e8d8-7a61-2289aeafd0cb@tarantool.org> (raw) In-Reply-To: <55d56797-f937-4088-fdba-7d9dfef51ae6@tarantool.org> I'll make review fixes and I'll send a corresponding letter later.Now I'd like to discuss a few unclear moments. >> err1 = >> err1:wrap(err) - add err error object as a reason for err1 object >> this call modifies err1 object and doesn't >> modify err object. > >> err1_parent = >> err1:unwrap() - remove the most recent error in error object, >> return it's parent. the call has no effect when >> there is no parent in given error object. > 3. If it modifies err1, then why do I need to assign a result to > err1? The same below. Let's start discussing this question with second :unwrap method. At first, some Lua variables point to error object (sic: not diag area); Therefore we need a way to return an unwrapped parent object to user. The implemented error:unwrap() method modifies an original error object(removing it's parent) and returns it's parent object. next_err = err:unpack() To make API consistent, I also return an error object in error:wrap(reason) method. This is the only reason for :wrap. We may get rid of it, if it is your strong opinion. >> +/** >> + * Wrap reason error object into given error. >> + * This API replaces box.error.last() value with an updated >> + * error object. > > 4. Why do you need to change the global error? And why is not > it mentioned in the docbot request? In many details my motivation is similar with (3.)th block: to make my API consistent. It is really important to enforce something taking a reference to parent error before unref(ing) it for self (for :unwrap) object. We would like to return reason for user, right? But self object make have the last reference to it. The delete method mustn't be called. Partially :wrap and :unwrap operations are constructors that introduce a new error. So changing box.error.last() seems for me reasonable. > 7. Unwrap does not allow to unwrap a leaf error. > But there is no API to determine if the error is > leaf. So a user can't determine when to stop calling > unwrap.> > I am talking about C public API which you have changed > here. A user can't check error->reason != NULL before > calling box_error_unwrap. I don't mind: I've had a draft with such implementation. Let's do it so. >> +err2 = nil >> + | --- >> + | ... >> +collectgarbage() >> + | --- >> + | - 0 >> + | ... > > 10. Nit: you could nullify all the errors at once, and call > collectgarbage. >What do you mean? I consciously clean up the errors and call the garbage collector in these places. If you put an extra printf in error_unref/destructor you'll see why this is important. (also see your 4th question - this is a coverage tests for this problem) >> + >> +s:drop() >> + | --- >> + | ... > 11. In the RFC you said, that IProto returns a list of error. Where > it is? I haven't implemented this yet. Kostya said that we make do it later.
next prev parent reply other threads:[~2019-08-06 7:56 UTC|newest] Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-08-01 11:13 [tarantool-patches] [PATCH v1 0/3] box: stacked diagnostics area in fiber Kirill Shcherbatov 2019-08-01 11:13 ` [tarantool-patches] [PATCH v1 1/3] box: rfc for stacked diagnostic area in Tarantool Kirill Shcherbatov 2019-08-05 21:16 ` [tarantool-patches] " Vladislav Shpilevoy [not found] ` <06bd2140-3d2b-4bc3-7bc4-5f3d293bf891@tarantool.org> 2019-08-06 20:50 ` Vladislav Shpilevoy 2019-08-07 23:27 ` Alexander Turenko 2019-08-08 20:46 ` Vladislav Shpilevoy 2019-08-08 23:29 ` Alexander Turenko 2019-08-09 19:25 ` Vladislav Shpilevoy 2019-08-12 20:35 ` Konstantin Osipov 2019-08-01 11:13 ` [tarantool-patches] [PATCH v1 2/3] box: stacked diagnostics area in fiber Kirill Shcherbatov 2019-08-05 21:16 ` [tarantool-patches] " Vladislav Shpilevoy 2019-08-01 11:13 ` [tarantool-patches] [PATCH v1 3/3] box: extend ffi error object API Kirill Shcherbatov 2019-08-05 21:18 ` [tarantool-patches] " Vladislav Shpilevoy 2019-08-06 7:56 ` Kirill Shcherbatov [this message] 2019-08-06 20:50 ` Vladislav Shpilevoy 2019-08-08 23:33 ` Alexander Turenko 2019-08-09 19:27 ` Vladislav Shpilevoy
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=c9b389bf-b1f5-e8d8-7a61-2289aeafd0cb@tarantool.org \ --to=kshcherbatov@tarantool.org \ --cc=tarantool-patches@freelists.org \ --cc=v.shpilevoy@tarantool.org \ --subject='[tarantool-patches] Re: [PATCH v1 3/3] box: extend ffi error object API' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox