On Thu, 03 May 2007 16:52:39 -0400, Havoc Pennington wrote: > Carl Worth wrote: > "cairo model" is probably confusing - write_to_png is a more typical and > essentially GError-equivalent model, while "cairo_t model" is the thing > that people mean is unique to Cairo. Yeah, we can get in a lot of confusion here whether we're talking about the merits of the approach in any particular library vs. the idea of applying that approach to new things. Anyway, I've learned a few new things, and found the discussion very useful. I hope you have as well. > Of course. The case where this would break is an object with its own > locks, e.g. DBusConnection. I'm just guessing that gvfs has its own > locks since I believe gnome-vfs did. Yes, internal locks would definitely change things a lot. I just generally don't believe in such locks, but that would be a whole separate thread... ;-) > There is an official GLib/GTK+ policy on this, which is that g_assert is > used only for bugs in the library, and return_if_fail is used for bugs > in the app. It is missing from the docs indeed. OK, that makes sense. > > I could imagine a consistent argument that all programmer errors > > should lead to an abort, (to force errors to get noticed and dealt > > with early). > > D-Bus in fact does this unless you disable it by env variable. It is > pretty controversial, though. Or at least there's a vocal group who > doesn't like it. > > Obviously I think said group is wrong ;-) And that makes a _lot_ of sense. I was a little confused with your rationale about programmer errors and then the discussion of g_return_if_fail. By the way, is that a compile-time or a run-tim environment variable? And what measures do you take to advertise it? > The big differences are 1) that it prints a message and 2) that all > behavior is undefined after a return_if_fail - GTK doesn't contain > handling code that frees resources or tries to recover or puts objects > in a no-op state as a result of the return_if_fail. Yup. I'd be glad to print in cairo if the programmer has asked for undefined behavior, but in that case I might as well just abort. > I don't think 2) really matters to library users, it only affects > library maintainer effort. Fair enough. My experience is that there's not much maintainer effort involved here. Implementing the no-op behavior is no harder than the moral equivalent of a single g_return_if_fail at the beginning of every entry point. And once you've got the no-op it's really easy to provide defined behavior without much extra code, (we just don't care what the state of the object is, except that it's destroy function should still work). > Well, if and only if the error is a programming error. That's the > problem, I don't want noisy in write_to_png, nor do I want noisy if the > error is something I should be handling or is something I'm legitimately > ignoring. I only want noisy about bugs. Fair enough. I think I could be convinced to accept code to make cairo fail with a message when the user asks for some undefined behavior. > What D-Bus does in a couple of cases (and GTK may have one or two > examples of this also, iirc) is that if you pass NULL to ignore the > DBusError/GError, it is noisy, and if you don't pass NULL it is not > noisy. However, this is only OK if passing NULL when an error occurs > amounts to a programming bug, i.e. when you are only allowed to pass > NULL if you know errors are impossible. If it would be legitimate and > normal for the error to be silently ignored in some cases, you can't be > verbose when one happens. I can't actually find an example in the code > quickly, but I swear we do this in a couple places. The stuff described in that paragraph seems ver confusing. :-) > For example say you pass nonsense values; the warning can be > "function_name: assertion failed: arg_foo != 0.0" so I look at my calls > to function_name where arg_foo might be 0.0. Like I said, I could see adding that, (and likely with an abort by default). The hardest part I think would be doing a good job about advertising this and advertising how to make it go away. > With cairo_t drawing code, you have to add temporary code to check the > status and print errors, then to figure out which function sets the > error you have to keep moving this temporary code around, then you have > to figure out what the status means once you know which function sets > the error status. Oh, yuck. You really should have complained to me about something like that. Nobody should ever have to keep moving temporary error-checking code around to find their mistake. That's awful. > Setting a break on _cairo_error is certainly easier (thanks for that > tip) but the reason I've never done that is that I did not know cairo > had an internal symbol that was always used to set errors ;-) for the > analogous situation in gtk, gdk_x_error, the warning when an X error is > received has something in it like "try setting a break on gdk_x_error" > even, or used to. Yeah, this is definitely an advertising problem. And if you'd be happy with the library aborting when it printed the message, then I'd be happy to print it. > D-Bus does this on glibc systems - GNU libc has a function to get a > backtrace for you. So the warning includes the function, the line, the > assertion or precondition that failed, and a trace. Thanks, I'll have to go look at that. > I think the cost of OOM handling in D-Bus is about a 30% code size > increase, and an absolute requirement to have substantial unit test > coverage that tests the OOM codepaths. For GTK+, I think it's an > intractable problem because the unit testing is intractable or at least > a monumental task. You could write the 30% code size but 10% of that > bloat would be broken and never get tested. And apps would never check > for OOM anyway. How does your D-Bus unit testing arrange to test all the OOM code paths? What we're doing in cairo, (only fairly recently, and thanks to Chris Wilson), is to run cairo's standard test suites but with a valgrind skin that Chris wrote to progressively inject malloc failures. The skin is quite general and is described here: http://lists.freedesktop.org/archives/cairo/2007-April/010343.html With this together with standard coverage tools, we can get very good testing of our OOM paths, but we're not really have to do any coverage or testing code that we wouldn't be writing anyway, (to test the normal paths). So, I'm not convinced that doing something similar with a codebase of similar complexity as GTK+ would necessarily be intractable. > Agreed. Please note, I don't have anything against the cairo_t model for > cairo_t (though I do wish it was not used for programming bugs, or at > least that optionally it was not used for programming bugs, or > optionally warning spam with function name, arg name, and assertion > failed could be printed for programming bugs). I only followed up to > point out that it isn't globally better and specifically I doubt it's > right for many things in gvfs. Fair enough. -Carl
Attachment:
pgpAI1gmwsF0X.pgp
Description: PGP signature