On 17 September 2013 00:08, Jakub Jermar <[email protected]> wrote: > On 09/15/2013 04:53 PM, Jakub Jermar wrote: > > For the record, I was watching the changes closely until about two weeks > > or so ago and I quite liked them. The whole series starts with a nice > > cleanup/handlification of the interface and the code and it seemed > > reasonable to me. Now, I still need to look at what was done in the most > > recent changes. > > Ok, I went through the changes as they stand now. It is possible that if > you throw the old API away again, new issues will arise, but this is a > list of reservations or comments that I have now: > > - use of magic constants, such as 4096 in valid_ptr() and formerly(?) > also >= 16 for valid handles >
I think 4096 is supposed to be PAGE_SIZE (I blame laziness to find the right header). The shifting of handles (16 was really arbitrary) was only used briefly to help with debugging. > - use of __builtin's > I thought I replaced everything except atomics. I will check that. > - async_exchange_begin(), does not require you to test for NULL return > value; as I was told at the camp, the exchange-consuming functions > should handle this gracefully; for example in vfs_connect_internal() > Ok. > - new synchronization primitives (such as in sleeper.h) (I guess Martin > already pointed this out) and fancy synchronization using atomics > instead of simple lock-based synchronization using the already > existing stuff (already pointed out by me :-) > sleeper.h is used to implement the pipe queue. It's been a while since I implemented it, but I think the reason was that it's impossible (or at least very difficult) to implement the same structure using stuff from <fibril_synch.h> As for atomics, I use them in places where lock-based synchronization would be too awkward for my taste. Still, I will revisit it once more, and see if I can do better. Off topic: I am fairly certain you could elegantly implement all of <fibril_synch.h> (or any other synchronization mechanism) just by using futex and sleeper, without directly touching internal details of fibril and async framework. > - use of printf() formatters; instead of using a portable formatter, you > appear to cast the variable to match the formatter; I found the > following cases: > (long long unsigned) service_id > (long long) de->size > http://trac.helenos.org/wiki/Printf could help > In my experience, casting the value is much more portable, not to mention understandable, than "portable formatters". But sure, I will change it. - inbox_entry implements its own linked list; why not use the already > existing list_t and its list_first() and list_next() to traverse the > list in your desired way? > > Because list_t feels too obscure to me. To me it's much easier to write linked list manipulation in-line, and I think the resulting code is clearer as well. Note that I did use list_t and list_foreach() until a few days back, but after recent addition of two more magical arguments to list_foreach() macro, I just didn't have the energy to go figure out how it works. That being said, I would not oppose changing this back to list_t... but for this one thing, I am not going to be the one doing the change. -- Jirka Z.
_______________________________________________ HelenOS-devel mailing list [email protected] http://lists.modry.cz/listinfo/helenos-devel
