GNOME Bugzilla – Bug 670023
Extract media ID for bootable ISO
Last modified: 2012-02-14 15:44:27 UTC
The main patch adds another string property that corresponds to the libosinfo's ID for the media. Also including a minor improvements patch.
Created attachment 207504 [details] [review] tracker-extract: Minor optimizations & CC warning fix
Created attachment 207505 [details] [review] tracker-extract: Extract media ID for bootable ISO In Boxes, we need to know which (libosinfo) media exactly are we talking about. Otherwise, we will have to do detection again.
Patches look fine to me, go ahead and apply Zeenix ;)
Fwiw I strongly disagree with the first patch except for the = NULL part. In the second one, maybe the property should be osinfo:MediaId instead of osinfo:mediaId to match osinfo:Installer. Or maybe osinfo:Installer should become osinfo:installer.
(In reply to comment #4) > Fwiw I strongly disagree with the first patch except for the = NULL part. Since variables add to memory usage, I was taught to avoid them unless they provide any value, which is not true in this case. Personal preferences, unless they are shared by others, can not be prioritized over functional improvements (even small ones). > In > the second one, maybe the property should be osinfo:MediaId instead of > osinfo:mediaId to match osinfo:Installer. Or maybe osinfo:Installer should > become osinfo:installer. Hmm.. I was following the style for 'oinfo:osId' since that is more like 'mediaId'. I'm good with both so its up to tracker devs.
(In reply to comment #5) > (In reply to comment #4) > > Fwiw I strongly disagree with the first patch except for the = NULL part. > > Since variables add to memory usage, I was taught to avoid them unless they > provide any value, which is not true in this case. Personal preferences, unless > they are shared by others, can not be prioritized over functional improvements > (even small ones). > Aside from the first hunk, there are *no* functional improvements in what you changed, so all that is left is "personal preference". I wouldn't have complained if there were other changes in these hunks.
(In reply to comment #6) > (In reply to comment #5) > > (In reply to comment #4) > > > Fwiw I strongly disagree with the first patch except for the = NULL part. > > > > Since variables add to memory usage, I was taught to avoid them unless they > > provide any value, which is not true in this case. Personal preferences, unless > > they are shared by others, can not be prioritized over functional improvements > > (even small ones). > > > > Aside from the first hunk, there are *no* functional improvements in what you > changed, so all that is left is "personal preference". I wouldn't have > complained if there were other changes in these hunks. 1. Reduction of memory usage *is* a functional improvement, though a very small one in this case. 2. Readability and simplicity is what matters and I don't see any readability improvement by usage of variable here: Simple predicate functions being called from an if condition.
Attachment 207504 [details] pushed as 252e33f - tracker-extract: Minor optimizations & CC warning fix Attachment 207505 [details] pushed as dba66b6 - tracker-extract: Extract media ID for bootable ISO
(In reply to comment #7) > 1. Reduction of memory usage *is* a functional improvement, though a very small > one in this case. This is just bullshit, start by learning how a compiler works. > 2. Readability and simplicity is what matters and I don't see any readability > improvement by usage of variable here: Simple predicate functions being called > from an if condition. Once again, personal preference.
(In reply to comment #7) > (In reply to comment #6) > > (In reply to comment #5) > > > (In reply to comment #4) > > > > Fwiw I strongly disagree with the first patch except for the = NULL part. > > > > > > Since variables add to memory usage, I was taught to avoid them unless they > > > provide any value, which is not true in this case. Personal preferences, unless > > > they are shared by others, can not be prioritized over functional improvements > > > (even small ones). > > > > > > > Aside from the first hunk, there are *no* functional improvements in what you > > changed, so all that is left is "personal preference". I wouldn't have > > complained if there were other changes in these hunks. > > 1. Reduction of memory usage *is* a functional improvement, though a very small > one in this case. Actually, Christophe is right from what I know too. You would be right in some sense because a stack register would be used to assign the variable for use later, but compilers are damn clever these days and this sort of thing is generally optimised out. So it has purely an aesthetic value. I thought similar things about using variables at the right scope level too, but again, compilers optimise those conditions too, so moving variables to lower scopes (which I was doing for years) really just helps the reader, it improves nothing else. Of course, your mileage may vary between compilers :) > 2. Readability and simplicity is what matters and I don't see any readability > improvement by usage of variable here: Simple predicate functions being called > from an if condition. I normally side with Christophe because at some point, if the code becomes complex you really want easily understood conditions like this: if ((is_foo && is bar && (has_sliff || has_sloff)) || is_readable) { ... } Instead of a bunch of function calls which is not so easy to read. -- Anyway, since you're both working together on this, I thought you just wanted to update the code there and I didn't really object.
(In reply to comment #10) > (In reply to comment #7) > > (In reply to comment #6) > > > (In reply to comment #5) > > > > (In reply to comment #4) > > > > > Fwiw I strongly disagree with the first patch except for the = NULL part. > > > > > > > > Since variables add to memory usage, I was taught to avoid them unless they > > > > provide any value, which is not true in this case. Personal preferences, unless > > > > they are shared by others, can not be prioritized over functional improvements > > > > (even small ones). > > > > > > > > > > Aside from the first hunk, there are *no* functional improvements in what you > > > changed, so all that is left is "personal preference". I wouldn't have > > > complained if there were other changes in these hunks. > > > > 1. Reduction of memory usage *is* a functional improvement, though a very small > > one in this case. > > Actually, Christophe is right from what I know too. > > You would be right in some sense because a stack register would be used to > assign the variable for use later, but compilers are damn clever these days and > this sort of thing is generally optimised out. So it has purely an aesthetic > value. > > I thought similar things about using variables at the right scope level too, > but again, compilers optimise those conditions too, so moving variables to > lower scopes (which I was doing for years) really just helps the reader, it > improves nothing else. > > Of course, your mileage may vary between compilers :) Compilers are always becoming smarter but I am too forgetful to remember all the optimizations they do. But thanks for educating me (although teuf's educating skills seem a bit rusty :)). > > 2. Readability and simplicity is what matters and I don't see any readability > > improvement by usage of variable here: Simple predicate functions being called > > from an if condition. > > I normally side with Christophe because at some point, if the code becomes > complex you really want easily understood conditions like this: > > if ((is_foo && is bar && (has_sliff || has_sloff)) || is_readable) { > ... > } > > Instead of a bunch of function calls which is not so easy to read. I agree actually but that is really not the case here, is it?