Hi Junio, On Mon, 22 Dec 2014, Junio C Hamano wrote: > Johannes Schindelin <Johannes.Schindelin@xxxxxx> writes: > > > For example, try to spot the error here: > > > > ... > > F(ALMOST_HAPPY, INFO) \ > > F(CANNOT_RECOVER, ERROR) \ > > F(COFFEE_IS_EMPTY, WARN) \ > > F(JUST_BEING_CHATTY, INFO) \ > > F(LIFE_IS_GOOD, INFO) \ > > F(MISSING_SOMETHING_VITAL, FATAL_ERROR) \ > > F(NEED_TO_SLEEP, WARN) \ > > F(SOMETHING_WENT_WRONG, ERROR) \ > > ... > > But that is not what is being suggested at all. I already said that > FIRST_SOMETHING is fine as a measure to initialize, didn't I? > > I am only saying that if you have a place to store customized level, > you should initialize that part with default levels and always look > it up from that place at runtime. It is perfectly fine for the > initialization step to take advantage of the ordering and > FIRST_SOMETHING constants. Thanks for clarifying, I was worried that you wanted to encode the severity levels explicitly (F(ID, LEVEL) instead of F(ID) in the correct order). The DRY principle also suggests that we should not encode the severity level in two ways (which would leave the door open for inconsistencies). That means that we should not initialize a static array of severity levels, but initialize the array using a loop. Okay, now that we have established that the initial ordering by severity makes sense, let's examine the initialization step. Basically, our approaches differ only in *when* to initialize that array of severity levels: you want to initialize it always, and I want to initialize it only when the severity levels are customized by the caller. Now, let's have a look how the fsck_options are currently initialized. My code follows the convention established with strbufs, argv_arrays, etc: there is a preprocessor definition (_DEFAULT, imitating the _INIT definitions) that allows us to initialize such structs very conveniently. Please note that no loop is required, and certainly no extra code has to be called to initialize the struct. We get away with initializing that array lazily in the fsck_strict_mode() function when we detect that it needs to be initialized, being populated by the very same function that provides the default settings before customization. This is a very robust setup as the knowledge about, say, the size of that array is confined strictly to fsck.c. However, if we had to change the lookup such that it uses an array always, we would have to introduce a function to initialize the struct, always, in particular we would have to find a place to call that initialization function in, say, builtin/fsck.c (actually, in every code path that calls into the fsck machinery). Arguably, the code would get more complex – introducing new call paths just to initialize the fsck_options struct – and I would argue further that there is no gain from an elegance, readability and maintenance point of view: whether the array is initialized lazily or not, it will be initialized the exact same way. All it means is that we have to introduce separate code paths because we would separate explicitly the initialization from the configuration step. Therefore, I do not believe that introducing an fsck_options_init() is what you would really want. An alternative would be to initialize the array at compile time – we would have to violate the DRY principle for that, repeating the severity levels many times over, and we could no longer confine the visibility to the message IDs to fsck.c because not only the size of the array of severity levels would have to be known to every user of fsck.h, but the exact default severity levels themselves, to be able to initialize the struct. But we could initialize the struct with a known set of settings via the _DEFAULTS definition that way. However, you already expressed slight disagreement with the preprocessor magic needed to initialize both the enum and the array of message ID strings from the same list in a way that lets the compiler ensure consistency; I am afraid that if I were to modify _DEFAULTS to populate the entire severity level array, the resulting code would find your utter contempt. I believe, therefore, that this is also not what you want. So that leaves only one alternative: to initialize a global array with the default severity levels at *some* stage. I have no idea what that stage would be, therefore we would have to either establish ugly, and DRY-violating, compile time initialization, or we would have to call a function before using any of the fsck machinery – but that is fragile: it is too easy to forget one call path and access an uninitialized array! Worse, even if we *had* a fully initialized array of default severity levels, we would still have to have an on-demand copy (i.e. lazy initialization) of said array lest we modify the global defaults in fsck_strict_mode()! Essentially, we would just *add* complexity to the current solution, not replace it with anything simpler. Therefore, I believe that you cannot be a fan of this alternative, either. In short, I still find it much more elegant to determine the default severity levels from the numeric enum values, and initialize the severity level array only on demand, than to introduce a separate call to initialize the array always, in particular since we would have to execute that initialization loop *all* the time in the latter case – even if we do not customize, let alone look up, any value – or clutter the code with ugly, ugly preprocessor constructs. And I hope that my arguments convinced you, too! Ciao, Dscho