| | <<<Back 1 day (to 2017/06/07) | 20170608 |
| sebras | tor8: do you have time for a few minutes of review? | 12:20.36 |
| | tor8: there are a few commits waiting on sebras/master. the first four or so you might have seen before, but I'm not entirely sure. | 12:21.00 |
| tor8 | at, the "squares" actually look like two dots in close proximity at 300dpi | 14:05.42 |
| | sebras: the first 4 I've reviewed and LGTM'd before | 14:08.23 |
| | avoid fz_var LGTM | 14:09.09 |
| | plug leak of PDF page object LGTM | 14:09.17 |
| sebras | the one for fz_bound_page() is not good though. | 14:09.42 |
| | tor8: it needs to be edited, but it also needs to be discussed. | 14:10.06 |
| tor8 | plug leak of opj decoder LGTM | 14:10.09 |
| | I dislike comments like "Does not throw exceptions." | 14:10.25 |
| sebras | tor8: why? | 14:10.34 |
| tor8 | these things may change, I think it's better to not document such cases because then we'll start relying on it and code then breaks | 14:10.46 |
| sebras | tor8: I thought we came to the conclusion that it was better to mention which functions we promise do not throw exceptions and what functions are allowed to and only document the former ones as they are much fewer..? | 14:11.15 |
| tor8 | if it takes a ctx argument, and is NOT called _drop_ then it MAY throw exceptions | 14:11.16 |
| sebras | tor8: ok, in that case we certainly need more fz_try()s in the code. | 14:11.38 |
| tor8 | I have no such memory... :) | 14:11.44 |
| | very likely, yes | 14:12.08 |
| | fz_close_device could throw exceptions | 14:12.22 |
| | fz_run_display_list ought to throw exceptions if the underlying device throws | 14:12.38 |
| | oh my ... it swallows and warns. I blame Robin! | 14:13.29 |
| sebras | tor8: fz_run_display_list() doesn't because it swallows them and just fz_warn()s. | 14:13.32 |
| tor8 | it could still throw in say fz_keep_stroke_state (which mallocs and clones an unshareable stroke state) | 14:14.23 |
| sebras | tor8: if we assume that fz_run_display_list() can throw, then all calls must be inside fz_try() (they currently aren't) | 14:14.52 |
| tor8 | fz_close_device can throw if the device throws | 14:15.12 |
| Robin_Watts | sebras: That's bad, I reckon. | 14:15.15 |
| | fz_run_display_list should totally be allowed to throw. | 14:15.27 |
| tor8 | I suspect catching errors in fz_run_display_list is to allow partially broken renderings in progressive mode? | 14:15.53 |
| Robin_Watts | The only time it shouldn't is if we are using the cookie, and telling it to count errors. | 14:15.55 |
| | tor8: Not just progressive mode. | 14:16.13 |
| sebras | tor8: I'm puzzled by one thing though: fz_drop_device() asks us to call fz_close_device() first, but if say fz_run_display_list() throws, should we really call fz_close_device() first? if we don't we get another warning from fz_drop_device() because it wasn't closed first. | 14:16.44 |
| tor8 | we should try making it not throw. we've tweaked this code and code around it quite a bit, so it might have unexpected behaviours now | 14:16.54 |
| | sebras: it's a bit of a hairy issue... close_device is intended to finish up a device when all has gone well | 14:17.44 |
| | such as in pdfwrite writing out the xref stream, etc | 14:17.52 |
| | but if something goes wrong, we would really like pdfwrite to delete the file and not leave broken things hanging around | 14:18.13 |
| | but we don't do that yet | 14:18.16 |
| sebras | tor8: yes, and if the device (or something completely different in the same fz_try()-block) fails the should fz_close_device() be called? | 14:18.25 |
| | if fz_close_device() is allowed to throw I would say that no, it cannot and should not be called. | 14:18.48 |
| tor8 | and we want to catch user errors where the API changed to require a close_device call to finish up (where the drop_device used to do it implicitly) | 14:18.49 |
| | hence the warning | 14:18.54 |
| | I've been considering adding a fz_abort_device, to clean up after exceptions | 14:19.43 |
| sebras | tor8: that would make sense to me. | 14:19.52 |
| | Robin_Watts: also, regarding the cookie, if the call to fz_close_device() actually does something to the output and this fails, should the cookie be updated? | 14:20.20 |
| | Robin_Watts: currently fz_close_device() takes no cookie. | 14:20.33 |
| Robin_Watts | fz_close_device throws, so any problems are reported there. | 14:20.59 |
| tor8 | so we have two cases: fz_try() { run-stuff(); close_device(); } fz_catch() { abort_device() } drop_device() or something like that | 14:21.14 |
| sebras | tor8: that seems plausible | 14:21.43 |
| tor8 | try { run(); close(); drop(); } catch { abort(); drop(); } is probably better | 14:22.02 |
| | but yes, something like that | 14:22.06 |
| Robin_Watts | tor8: Why not just: fz_try() { run-stuff(); close_device(); } fz_always() { drop_device(); } fz_catch (rethrow();} | 14:22.08 |
| tor8 | tor8: you mean to implicitly call abort_device if close_device has not been called? | 14:22.32 |
| sebras | Robin_Watts: in that case I'd like drop_device() to not complain if close_device() is not called in case of error. | 14:22.36 |
| Robin_Watts | Calling drop without having called close is implicitly aborting. | 14:22.38 |
| tor8 | Robin_Watts: yeah, that would work too | 14:22.52 |
| sebras | tor8: you mentioned that we have the warning to catch users that adhere to the API before the change. how would I know what those users are? | 14:23.26 |
| tor8 | Robin_Watts: sebras: something I'd like to do here is for pdfwrite (and other output writing functions) to write to a tempfile and rename at the end in success | 14:23.36 |
| Robin_Watts | sebras: I'm happy for it to warn (in release modes at least) | 14:23.36 |
| sebras | tor8: externally I can't look at the code but inside mupdf I can. | 14:23.38 |
| tor8 | and remove the tempfile in case of errors | 14:23.42 |
| | so we don't clobber existing files when things go wrong | 14:23.51 |
| Robin_Watts | s/release/debug/ | 14:23.56 |
| | tor8: makes sense... except that using a duff filename to start with will then not throw an error until the end. | 14:24.20 |
| tor8 | Robin_Watts: it might throw an error on the final rename (which would happen inside the fz_close_device call) | 14:24.43 |
| | if the permissions are wrong, etc | 14:24.52 |
| Robin_Watts | tor8: Indeed. | 14:24.57 |
| | In a way, I'd far rather get that right at the start. | 14:25.14 |
| tor8 | but better than starting to overwrite an existing document, and blowing up on something else and destroying the user's data | 14:25.16 |
| Robin_Watts | If people want to do the 'use a temp file, then rename' strategy, they can implement that themselves. | 14:25.28 |
| tor8 | Robin_Watts: yeah. but then we should start doing that in our viewers, apps, and tools :) | 14:25.51 |
| Robin_Watts | That sounds more reasonable than doing it internally, to me. | 14:26.06 |
| tor8 | it's why I've been holding off ... it seems a bit weird to do it internally | 14:26.19 |
| | otoh, it is notoriously difficult to get right (especially in java/js/etc) so doing it internally could help users not cock up | 14:27.21 |
| sebras | tor8: wait.. we are talking about pdf_new_pdf_device() here, right? from a brief scan of the source it seems as if it doesn' | 14:29.20 |
| | t open a file. | 14:29.23 |
| | but instead allocs a dev->buffer which seem to be populated? | 14:29.35 |
| | tor8: the saving happens in pdf_save_document() which doesn't take a fz_device | 14:31.29 |
| | AIUI that means that fz_close_device() isn't involved in the file handling and so no longer inhibits altering the device usage..? | 14:33.09 |
| tor8 | sebras: like I said, this is all up in the air :) | 14:43.19 |
| | sebras: I'm more talking about the fz_document_writer interfaces here, but there are parallels in the device interface | 14:43.52 |
| | I think the general idea we can/have to rely on is that not calling close_device means an aborted job | 14:44.16 |
| | in both the device and writer interfaces | 14:44.27 |
| | Robin_Watts: the file juggling is also different on unix-ish and windows platforms | 14:44.50 |
| | and creating temp files doesn't have a reliable cross platform interface either | 14:45.04 |
| Robin_Watts | tor8: This sounds more like "helper function" territory to me rather than trying to shoehorn complex and not entirely consistent behaviour into existing code. | 14:46.20 |
| tor8 | Robin_Watts: yes. | 14:47.38 |
| sebras | tor8: so adding ABORT to fz_new_derived_document_writer(CTX,TYPE,BEGIN_PAGE,END_PAGE,CLOSE,ABORT,DROP) and then implemting this everywhere and have it be implicitly called by fz_drop_document_writer() unless the device was alreayd closed. | 14:59.26 |
| | and keeping the warning. at least in debug builds. | 14:59.51 |
| Robin_Watts | I'm not sold on adding abort. | 15:00.33 |
| | I don't see how it gains us anything. | 15:00.39 |
| | drop without close == abort. | 15:00.51 |
| tor8 | Robin_Watts: I think you've sold me on that argument too :) | 15:01.09 |
| Robin_Watts | Suppose you *do* add abort... if we drop without close, how does that differ? | 15:01.25 |
| sebras | ok, so it all should happen inside DROP? | 15:01.27 |
| Robin_Watts | sebras: drop just drops. Free stuff and drop it. | 15:01.50 |
| tor8 | Robin_Watts: the only benefit would be being able to tell apart the abort and user error case | 15:01.52 |
| sebras | it wouldn't differ. | 15:01.57 |
| tor8 | as in you'd need to call either close or abort explicitly, or get a warning when you drop | 15:02.11 |
| Robin_Watts | everything clever happens in close :) | 15:02.12 |
| sebras | Robin_Watts: then we can't close the device as clever tings might throw. | 15:02.36 |
| | s/tings/things/ | 15:02.43 |
| Robin_Watts | Screw the user error. Either call the interface properly, or stuff won't work. | 15:02.47 |
| tor8 | Robin_Watts: fair enough. | 15:02.56 |
| Robin_Watts | sebras: I'm failing to see the issue here. Can you give me an example. | 15:03.03 |
| tor8 | sebras: fz_close_device can throw. fz_drop_device cannot (because of the need to use it in java finalizers, etc) | 15:03.33 |
| sebras | tor8: agreed. | 15:03.47 |
| | tor8: but you mentioned that you wanted clever things to happen when a device fails, such as removing files. | 15:04.06 |
| | tor8: would those clever things not go into close? | 15:04.17 |
| tor8 | sebras: no. because when we call close device it doesn't know that things have gone bad. | 15:04.35 |
| sebras | tor8: or you'd leave that to the called that mgith catch the fz_throw() and have to resolve it? | 15:04.44 |
| tor8 | which is why I briefly advocated an abort call | 15:04.47 |
| | but as robin said, leaving cleaning up files and doing safe saves up to the user is a better idea | 15:05.15 |
| sebras | tor8: ok, how would we know in drop that things have gone bad? | 15:05.17 |
| tor8 | sebras: we would know, because the 'close' function has *not* been called | 15:05.33 |
| | the same way we now print a warning if you drop without calling close | 15:05.49 |
| sebras | tor8: ok, but if we don't want to do clever things then we don't need to know. | 15:06.12 |
| | tor8: if there's a file open we simply close it and ignore any error. | 15:06.24 |
| | especially since drop may not throw... | 15:06.33 |
| tor8 | yeah. if we leave it up to the user to clean up then we can just ignore doing any special "abort" handling | 15:06.55 |
| | and in drop_device we just close the file or whatever, and let the caller clean it up | 15:07.11 |
| | or in the pdf_device case, we never write the file to start with | 15:07.27 |
| | sorry, pdf_document_writer case | 15:07.51 |
| | since the call to pdf_save_document happens when we close the document writer | 15:07.56 |
| | and if we get an error somewhere along the way, we never call pdf_close_document_writer | 15:08.18 |
| | make sense? | 15:08.25 |
| sebras | tor8: that seems reasonable. | 15:08.28 |
| dinamic | tor8, i pushed my rust bindings the other day, i copied a few snippets of texts from mujs site, take a look and tell me if you want to change anything.. | 15:10.01 |
| | tor8, https://github.com/hean01/mujs-rs and https://docs.rs/mujs/0.0.2/mujs/ | 15:10.33 |
| sebras | Robin_Watts: just for completeness: I refuse to give you and example. ;) | 15:11.38 |
| tor8 | dinamic: I don't know enough rust, but is runtogetherlowercasewords the usual naming scheme? | 15:11.42 |
| Robin_Watts | sebras: I refuse to read your non-existent example | 15:12.10 |
| tor8 | dinamic: I have no objection if you want to rename functions to be more in line with usual rust conventions. | 15:12.16 |
| | dinamic: also: cool! | 15:12.20 |
| dinamic | tor8, yea, the api functions are pure js_orginalname copeis for now. I will probably to a higher level object with more rust like api | 15:13.43 |
| tor8 | if rust supports runtime type information (or compile time macros to do it) you could auto-wrap functions and their arguments, etc | 15:14.53 |
| sebras | dinamic: your documentation mentions mujs being under the AGPL, but the license has recently changed: http://dev.mujs.com/ | 15:15.07 |
| tor8 | and call the js_tostring on arguments and return values to call any rust function | 15:15.21 |
| | anyway, gotta go. back in a couple of hours. | 15:15.45 |
| dinamic | sebras, ah ok, would it be compatible to make the rust binding library GPLv3 | 15:17.44 |
| sebras | dinamic: as far as I know MuJS is now under the ISC license which according to wikipedia is GPL compatible: https://en.wikipedia.org/wiki/ISC_license | 15:24.19 |
| | dinamic: actually referring to FSF might be evern better. :) https://www.gnu.org/licenses/license-list.html#ISC | 15:24.47 |
| | dinamic: be sure to use commit 14b9aab19395c6aec0cb53205c5d6d6da7d057f5 where the license change takes place though. | 15:26.27 |
| | dinamic: (or later of course) | 15:26.36 |
| dinamic | sebras, for now, mujs-rs has mujs upstream as a git submodule which will be kept updated | 15:29.13 |
| sebras | dinamic: sounds quite reasonable to me. | 15:44.21 |
| | Robin_Watts: ugh. do you mind reviewing eb2f7358ddc02198483fc43a0d69842f0b3d2ca1? | 15:44.38 |
| | Robin_Watts: I accidentally pushed it to master without getting a LGTM from tor. | 15:44.51 |
| | (I got LGTM for the one's above and below) | 15:45.02 |
| | Robin_Watts: or help me in removing it. | 15:45.14 |
| Robin_Watts | sebras: Just a mo... | 15:45.29 |
| | Urm.... | 15:46.51 |
| | I thought we agreed that fz_close_device DID call exceptions ? | 15:47.07 |
| | likewise fz_new_list_device | 15:47.18 |
| | hmm. | 15:48.26 |
| | I don't like the fact that fz_run_display_list doesn't throw. | 15:48.54 |
| | In fz_prepare_t3_glyph, I don't see why we set dev = NULL (though that's true in the current code too). | 15:52.47 |
| | I also don't see why fz_close_device is in the always rather than the try. | 15:53.03 |
| | So... shall we back that one out? | 15:53.26 |
| | Updated history pushed to robin/master.... | 15:54.32 |
| | force pushed to golden. | 15:55.47 |
| | and cluster sorted. | 15:59.19 |
| | sebras: I think that's it. | 15:59.25 |
| sebras | Robin_Watts: excellent, thanks. | 16:16.10 |
| | Robin_Watts: I comitted it by accident and I'm sure tor8 reviewed it and that this triggered the previous discussion. | 16:16.42 |
| | Robin_Watts: so yeah, I'll be redoing that. | 16:16.55 |
| Robin_Watts | sebras: No worries. easily done, easily fixed. | 16:17.09 |
| sebras | tor8: https://news.ycombinator.com/item?id=14507597 might be interesting to you? | 16:34.33 |
| dinamic | i might have found a bug in mujs, or i'm just doing it all wrong, see https://pastebin.com/ESWhQHtL | 16:45.53 |
| | Is that a correct assumtion that i can remove the item from the array ? | 16:46.13 |
| | The result of that test is that there is still 3 items in the array | 16:46.45 |
| | avih: ^ ideas | 16:47.45 |
| avih | dinamic: the length is the highest "index" populated +1 | 16:49.40 |
| | so it should be 2 as index 2 is populated | 16:49.48 |
| | should be 3 * | 16:49.54 |
| | (where "index" is 0 or positive integer) | 16:50.19 |
| dinamic | oh | 16:50.36 |
| | hmm | 16:50.39 |
| avih | you could do var a = []; a[100] = undefined; and a.length will be 101 | 16:51.15 |
| dinamic | if i remove a[100] will length be 1 ? | 16:51.37 |
| avih | 0 | 16:51.42 |
| dinamic | ah right | 16:51.51 |
| avih | but forEach, join and others skip nulls and undefined and unpopulated | 16:52.37 |
| dinamic | im confused, in my example, if i remove elemnt index 3 length will be ? | 16:53.47 |
| avih | you don't have index 3, but if you remove index 2, then the length will be 2 | 16:54.04 |
| | [0,1,2] each index is populated with its own value here | 16:54.35 |
| dinamic | i change my code to remove index 2, getlength still report 3 ? | 16:55.10 |
| | thats why im confused | 16:55.19 |
| avih | hmm.. shouldn't be the case. maybe a bug someplace. you could test it in a browser to verify | 16:55.45 |
| | wait, i don't think you can reference this array at all | 16:56.21 |
| | you don't assign it to anything, it's just sacrificed to the gods of gc | 16:56.45 |
| dinamic | oh | 16:56.55 |
| avih | do "a = [1,2,3]" and then getglobal(J, "a") and then delindex 2 of the item at -1 | 16:58.14 |
| dinamic | i have other tests that works as expected doing "[]" | 16:58.23 |
| | just tested it as you described. no change.. | 16:58.41 |
| avih | ... too little information to reference.. | 16:58.45 |
| dinamic | let me play in a browser | 16:58.50 |
| | avih, why item -1 after getglobal() ? shouldn't it be stack pos 1 after a getglobal() | 17:02.19 |
| avih | depends what you had in stack before. but -1 is always the topmost item | 17:02.43 |
| | i'm not sure what the stack contains outside of a callee javascript c function | 17:03.50 |
| dinamic | anyway, 0 in my case, but still no success | 17:05.26 |
| avih | care to post complete c code? | 17:06.16 |
| dinamic | https://pastebin.com/HNLxAELK | 17:06.39 |
| avih | (i.e. minimal complete program) | 17:06.42 |
| | it's not c ... | 17:06.59 |
| dinamic | replace state. with js_ does it look ok ? | 17:07.38 |
| | let me build a c lib and make a test c prog of same | 17:07.54 |
| avih | (i can follow the code, but it clearly uses some wrapper to the actual mujs lib, which i prefer to avoid while analyzing issue) | 17:09.16 |
| | dinamic: try replacing 1 with -1 at delindex and getlength | 17:12.26 |
| dinamic | that thows a typeerror | 17:12.46 |
| | my c program shows the same problem | 17:12.55 |
| | https://pastebin.com/FJCRiNTA | 17:13.02 |
| avih | what? | 17:13.08 |
| dinamic | length is still 3 | 17:13.31 |
| avih | dinamic: i can reproduce (at the mujs REPL). maybe delete doesn't change length? that sounds weird to me | 17:15.54 |
| | what does it do in a browser? | 17:16.00 |
| dinamic | i cant find eqvalent action | 17:16.09 |
| | in my browser, no del function at al.. | 17:16.21 |
| | its weird, delindex calls delproperty and is an wrapper convinience function of mujs | 17:17.16 |
| | as i understand | 17:17.24 |
| | of the code.. | 17:17.32 |
| avih | mujs seems to behave correctly. in javascript it's delete arr[3], but it doesn't delete the item or truncate the array | 17:18.04 |
| | or arr[2], same. it's not deleted | 17:18.21 |
| dinamic | maybe arrays are not configurable eg DONTCONF ? | 17:19.03 |
| avih | i wouldn't think so. best look at the implementation of js_delindex and see what it does | 17:19.33 |
| dinamic | anyway a correct test is delindex() then try getindex() | 17:19.36 |
| | delindex -> delproperty | 17:19.44 |
| | it only does a delproperty | 17:20.00 |
| avih | except it doesn't remove the property from my experiment. neither in a browser | 17:20.02 |
| dinamic | so.. | 17:20.03 |
| avih | works on an object, not on an array. which is weird. | 17:20.20 |
| dinamic | huh | 17:20.27 |
| | array_prototype is an object_prototype as i could read the code.. | 17:20.48 |
| | gah | 17:20.50 |
| | :) | 17:20.54 |
| avih | ... not sure. | 17:21.11 |
| dinamic | avih, check this.. https://pastebin.com/5uPJrUrB | 17:24.20 |
| | that works removeing the index | 17:24.26 |
| | 2 | 17:24.30 |
| | tostring will print undefined if uncomment js_delindex | 17:24.48 |
| | but whats up with the stack ? | 17:24.59 |
| | ah ok, change -1 to 1 and it looks ok.. | 17:25.54 |
| | and also works ok | 17:26.11 |
| | ok i rewrote my test to do proper test of js_delindex, length prop is unchanged. if thats a bug or not is something for ... tor8 ? | 17:30.02 |
| tor8 | avih: dinamic: var a = []; a[100] = 5; a.length is 101. delete a[100]; a.length is still 101. it only ever grows the length, never shrinks. | 19:35.57 |
| | dinamic: avih: on linux the libmozjs-24-bin package has the spidermonkey command line interpreter "js24" | 19:42.52 |
| dinamic | got that | 20:00.01 |
| | tor8, if a do js_loadstring(J,"x", "[1,2,3]") and a call i assume that i get a anonymous array on top of stack, do i have to worry about gc ? | 20:01.43 |
| | as long it is on stack ? | 20:01.58 |
| | many of my test cases does that semantics, and i just want to be sure its vali | 20:02.42 |
| | d | 20:02.43 |
| tor8 | dinamic: yes, that should be okay. a "script" function (as created by js_loadstring/loadfile) will return the value of the last expression evaluated | 20:42.32 |
| dinamic | perfect | 20:42.54 |
| tor8 | you only need to worry about object lifetimes for js_tostring pointers | 20:43.05 |
| dinamic | and those are taken care about at the rust wrapper | 20:43.22 |
| tor8 | they are only valid as long as the stack slot they came from is not overwritten | 20:43.28 |
| dinamic | so no worries there.. copies travels to rust space | 20:43.32 |
| | i havent read the specs, but how does a number primitive differ from number object ? object prototype or not ? | 20:45.07 |
| tor8 | a number primitive is a value with type 'number' | 20:45.34 |
| | a number object is a value with type 'object' that wraps a number | 20:45.52 |
| | js will automatically coerce a value with type 'number' to an object by creating a new object wrapping the number when you try to use the value as an object | 20:46.27 |
| | and vice versa by calling object.valueOf() when it needs a number and you have an object | 20:46.45 |
| | a number "primitive" is not an object, and does not have a prototype at all | 20:47.25 |
| | there are 4 basic types in javascript, and two special values: boolean, number, string, and object. and magic values 'null' and 'undefined' | 20:48.33 |
| | and there are several internal classes of objects (which have nothing to do with the prototype, and is not really exposed in the language) | 20:49.56 |
| | object, array, math (the Math global object), json (the JSON global object), regexp, string, number, boolean, function, etc are the "classes" | 20:50.50 |
| dinamic | perfect, just as i assumed. | 20:51.08 |
| tor8 | type coercion can be problematic, which is why lots of people suggest always using the '===' operator for comparisons | 20:51.44 |
| | because otherwise JS will try to convert them to equivalent types before comparing | 20:51.58 |
| dinamic | will js_equal() do that on primitives ? | 20:55.29 |
| tor8 | dinamic: yes. js_equal is equivalent to '==' | 21:56.01 |
| | js_strictequal is equivalent to '===' | 21:56.08 |
| | Forward 1 day (to 2017/06/09)>>> | |
Log of #mupdf at irc.freenode.net.