On Mon, Apr 20, 2009 at 01:17:43PM +0200, Carl-Daniel Hailfinger wrote:
On 20.04.2009 12:12, Luc Verhaegen wrote:
Flashrom: board_enables: reconstruct table.
This patch restores the pciid based board matching table. It makes this table readable and hackable again, and the only disadvantage is that the right margin is way beyond the rather dogmatic 80. All 0x0000 pci ids have been string replaced by 0 to more easily spot missing ids, and extra comments have been added to explain how the various entries are used.
Signed-Off-By: Luc Verhaegen libv@skynet.be
The extra comments are good, but "coreboot bios" in such a text really hurts. Please change the word BIOS to all-caps and don't refer to coreboot as a BIOS. Thanks.
These are the lines in question:
* The coreboot ids are used two fold. When a coreboot bios is installed, it * uniquely matches the coreboot board identification string. When a legacy * bios is installed and when autodetection is not possible, these ids can be * used to identify the board through a command line argument.
It is clear and concise.
Let's take the other option: "when coreboot is installed" What does that mean? Did the user check out the tree, did he install some of the utils, or did he install a coreboot bios image into his rom?
Your statement is more about politics than anything else.
The documentation part can be committed after one more iteration.
I would never have altered these comments if it wasn't for the previous flamewar. For those just tuning in, this is the thread in the mailarchives: http://www.coreboot.org/pipermail/coreboot/2009-January/044086.html
The rest of the patch makes board entries unreadable.
We had this one before. It makes it unreadable for you. But not for everyone. Today, with widescreen panels, it is easier to widen your editor and see the whole line under the condensed table (and currently, see the whole table!), than it is to see more than 2 entries under the unraveled one. All you have to do is get over the 80chars per line dogma. This is one of the few situations where imho it should be allowed, it is actually the only one that i can name where i would defend it.
In the past few weeks, we had quite a few people in #coreboot who wanted to add support for their boards. The presence of struct member names helped immensely to explain how to add board enables. Some even figured it out from similar entries. Lowering the barrier for possible future developers is very important.
What makes you so certain that it is this change that made people add 4 entries in 3 months? Couldn't that be because flashrom just is getting used more? And besides, this growth is not significantly faster than before.
Since IIRC you said some time ago that the multiline layout hinders your workflow, feel free to submit any new board enables in the single-line layout. I'll fix them up.
Why is my workflow relevant? Do you really think that i have all my editors open at 160 chars all the time? My you are referring to my statements about being able to oversee everything more quickly with a condensed table. This is just common sense, everybody will have to acknowledge that one.
What about your workflow? How often have you added entries to this table?
Actually, since i have dealt with such tables in my graphics drivers before (and have done so for 5 years no, hence me creating this table in the first place); How often have you dealt with pci id based matching tables?
What will you do when it reaches a hundred or so entries? Will you be happy with a 1400line mess? Do you think that this table will stop growing at some point?
How many actual board enable functions will there be with multiple entries in the table? How much space will the actual enables take compared to the table?
How will people tell whether something similar has already been added?
Will you impose some entry ordering scheme (which, in light of likely future table growth, is actually a useful change)?
It rapidly becomes non-obvious when you don't have a tight table. And i know what you are going to say here: just make sure that they are added in whatever order already. And my answer to that is: you can't. Left or right, entries will be added that aren't ordered, and afterwards no-one will spot them being unordered anymore. In a tight table, you spot them right away and can fix it easily on the next iteration.
It really is creating a situation where you are unable to see the forest through the trees.
Also, you originally suggested to remove lines like = 0x0000; or = NULL; This is a very dangerous path. People will only see the last 2-3 entries, and never look beyond. Remove a line, and it will not be thought of for the next entry. This is simply human behaviour.
What you then end up with is a whole range of boards where just a single pair of main ids, the coreboot ids, the string and the enable will be created. No autodetection will be provided any more, as the trivial addition of yet another argument-specified-only board is of course obvious for the person who added the board enable to begin with.
The end result then is that all users will just randomly go through the board names for ages, all the time, some of which will work, as the amount of pci devices really isn't that great. This is quite a dangerous situation and a really bad user experience.
With a condensed table, people are forced to pad, and will put more thought into their entries and will feel obliged to add subsystem ids. This in turn leads to more autodetection and less people blindly forcing boardnames, and in general a better user experience.
So maybe making it "easier" for people to "understand" what the different entries are for (which actually boils down to being able to ignore how the entries are used), is not a goal that should be pursued above other goals. For existing developers, at least those willing to not religiously stick to dogmas when it makes sense, the condensed table is the better option.
Luc Verhaegen.