On Sun, Nov 15, 2009 at 19:13, David Brownell <davi...@pacbell.net> wrote: > On Sunday 15 November 2009, Michael Bruck wrote: >> >>> The macro is right above the table. > > Yep. At least, *now* it's right above.
This is the original commit: http://openocd.git.sourceforge.net/git/gitweb.cgi?p=openocd/openocd;a=blob;f=src/target/arm11.c;h=11e376a6e6b935e9107f60c1c1d01721d0da924d > >> >> The problem with this sort of macros is that > > ... it doesn't really add anything. Even though > it *is* right above that table, you still have to > think more about it than when you see standard > initializer syntax (".field = value,"). The lines > above the top of the table are not necessarily on > the screen, or in your memory. It avoids redundant code and focuses the attention on the important parts. >> >> we have >> >> a dozen targets and having a dozen different >> >> macros gets a bit tedious. The arm11 macros >> >> does show that certain things are tedious in OpenOCD >> >> speak, but hopefully we can address those on >> >> in a more general fashion. >> > >> > This seems highly unlikely, quite a few contributions here lately >> > center around enforcing antiquated bloated syntax. > > Instead of "antiquated bloated" I'd say "standard and > universally understood". We're not exactly talking > about a COBOL level of verbosity here. ;) struct foo and foo_t are both universally understood. "struct foo" however clutters the code, especially in long function parameter lists, and distracts from the underlying algorithms/parameters. What you call "standard" is just other people's programming preferences. > Did you ever see the original Bourne shell sources? > The ones that abused CPP to make the language look > like Algol-60 instead of C? Now *that* was centered > around enforcing antiquated syntax ... Your patch is purely about enforcing arbitrary syntax rules and in the process adding redundant code that obscures what is going on. For example consider: ... ARM11_HANDLER(init), ARM11_HANDLER(foo), ARM11_HANDLER(bar), GENERIC_ARM_HANDLER(dog), ARM11_HANDLER(moo), ARM11_HANDLER(close), ... While not elegant it makes it immediately clear that dog is derived from the generic arm handler. On the other hand in the macro-free version it is far easier to overlook: .init = arm11_init, .foo = arm11_foo, .bar = arm11_bar, .dog = arm_dog, .moo = arm11_moo, .init = arm11_close, >> > The patch does not >> > improve legibility it merely reflects certain contributer's personal >> > distaste for macros. >> >> This should be contributors', plural is probably warranted here. > > It's also a fairly standard policy in most projects: > avoid syntactic sugar macros. To be used, they need > first to be learned ... and that's effort that could > be more usefully applied to other tasks. And to be > used, they also need to be maintained ... likewise. The reason why I did not plug arm11 into the existing arm infrastructure and why we implemented etm as a tcl script instead of using etm.c is because, beside being mostly undocumented, most of the openocd code in these areas is a programming hazard full of endless repetitive code that obscures the underlying algorithms. Reverse engineering what these functions do just costs too much time. Take for example: fields[0].tap = etm_reg->jtag_info->tap; fields[0].num_bits = 32; fields[0].out_value = reg->value; fields[0].in_value = NULL; fields[0].check_value = NULL; fields[0].check_mask = NULL; fields[1].tap = etm_reg->jtag_info->tap; fields[1].num_bits = 7; fields[1].out_value = malloc(1); buf_set_u32(fields[1].out_value, 0, 7, reg_addr); fields[1].in_value = NULL; fields[1].check_value = NULL; fields[1].check_mask = NULL; fields[2].tap = etm_reg->jtag_info->tap; fields[2].num_bits = 1; fields[2].out_value = malloc(1); buf_set_u32(fields[2].out_value, 0, 0, 0); fields[2].in_value = NULL; fields[2].check_value = NULL; fields[2].check_mask = NULL; This piece of digital diarrhea is no doubt "standard and universally understood" C code. Unfortunately it is also an obstacle when someone tries to read the surrounding algorithm. If that block would say: OUT_FIELD(fields[0], 32, reg->value); OUT_FIELD(fields[1], 7, reg_addr); OUT_FIELD(fields[2], 1, 0); it would be macros you might have to look up once when you deal with openocd for the first time, but that is a tiny price to pay for immediately seeing the relevant parts of the action. Btw. I added a small typo to the long version above that would not be detected by the compiler and is difficult to make with the macro version. Do you think someone would spot that when checking this in as a patch? And if it is not a typo but an obscure special case for a given CPU, would you, when inheriting that code for another CPU, notice a small difference that is buried under that pile of repetitive code that you skim over at best? Or to take the ARM11_HANDLER example, would you quickly spot the error if you forgot to switch arm_bar to arm11_bar when you replace the function with a more specialized version? The point is, the arm11 code doesn't use macros for fun, it uses them for clarity and to avoid trivial mistakes. And unlike the OUT_FIELD example it uses them only where functions are impractical or impossible. My opinion is that a general distaste for macros (or any C code that deviates from K&R) is not a good reason to override these design decisions. Michael _______________________________________________ Openocd-development mailing list Openocd-development@lists.berlios.de https://lists.berlios.de/mailman/listinfo/openocd-development
