> > Hi, > > On Wed, Jul 04, 2018 at 11:21:56AM +0200, Christophe Fergeau wrote: > > On Tue, Jul 03, 2018 at 05:10:23PM +0200, Christophe de Dinechin wrote: > > > There is a lot of debate, distributed across a large number of patches, > > > regarding similar SPICE and glib facilities. For a number of things, > > > there are two sets of parallel APIs with slightly different behaviour. > > > This is undesirable, as it introduces confusion. > > > > > > Pros: this lets us customize the behavior for SPICE > > > > > > Cons: the SPICE macros are less documented, and not obvious to tell how > > > they differ from the glib counterpart. > > > > > > > > > As I wrote in a response to a patch comment, I personally value > > > consistency relatively high, so if a file currently uses > > > spice_return_if_fail, that’s what I will use in that file. While I may > > > agree that using g_return_if_fail in general could be preferable to > > > spice_return_if_fail, I would like > > > > > > a) to avoid having repeated and unproductive comments about “why not > > > use the glib version” for every patch that touches one of these > > > macros. > > > > > > b) to have a clearer understanding of what the benefits of the SPICE > > > and glib variants are (I somewhat understand the difference, my > > > question is more about whether there is real value add to the SPICE > > > variant or not). > > > > > > > > > I had started explaining my preferences here, but I realize it’s > > > probably better to gather everyone’s opinion first. Please share your > > > preference, e.g. should we switch to glib wholesale, piecemeal, not at > > > all, on a per-macro basis, etc, and the rationale behind your > > > preference. Meanwhile, I’ll do some additional experiments to solidify > > > or change my own preference. > > > > Initially, spice-server/spice-common had no glib dependency, so > > spice_warning & friends made sense. Then they started to depend > > on glib, so some of the stuff they contained became redundant > > with what glib provides. Then spice_log was patched to go > > through g_log, while retaining its slight behaviourial > > differences, mostly for backward compatibility. At this point, > > the plan was to slowly get rid of spice_* and to move to glib > > logging. Which is why I push for using glib logging when a > > patch tries to use the deprecated spice logging API. If the > > inconsistency within a file is a problem, then the file can be > > switched to the new API first, and the new code can use the > > glib API and everything is consistent. Keeping using spice > > logging API for the sake of consistency is not going to make > > the situation better. > > > > Christophe > > I do like the approach of sanitizing the file(s) that the > following up patch will touch. Should be easier to test and > review but would take several patches to sanitize the code, etc. > > Now might be a good time to start with this as we just released > spice-gtk and a release of spice is not too far either. > > And, yes, I'm in favor of removing spice related functions/macros > that there is a glib equivalent. > > Cheers, > toso > In some way I can say I agree with everybody but I think Victor introduced the real "key" to the question: the word "equivalent". Let me explain. This discussion started specifically with the spice_critical/g_critical APIs. Now are they equivalent? Well, one call a backtrace function and shows the backtrace on standard output the other not. So, they are not 100% equivalent. Of course we can discuss on the value of the additional/different behaviour but are not equivalent. Another question is however "Are we going to use g_critical as g_critical?". It sounds a tricky question. Let say that a new person starts to look at the code and knows GLib. He see g_critical and think "well, this by default log a critical warning and continue" but instead on Spice is always fatal. This confused, and I think is not to blame, c3d. teuf state clearly that spice_critical calls abort. So clearly changing spice_critical to g_critical confuse and does not help new comers. Maybe, as expected to call abort, spice_critical should be replaced by g_error using Glib in its default expected behaviour (by extension g_return_if_fails should be replaced by a g_assert or similar) ? Frediano _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel