![]() | ||||||||||
Messages in this thread |
On 5/5/07, Artem Bityutskiy <dedekind@infradead.org> wrote: > On Sat, 2007-05-05 at 19:18 +0530, Satyam Sharma wrote: > > Well, you're developing / maintaining this right now, so it's your > > call. Though I bet most people would find keeping that list_add_tail > > local to scan.c more tasteful. > > I do not think so. If you are interested, try to find "UBI take 2" > patches in lkml. Look how it looked liked. It consisted of many > independent units and units could access other units _only_ via > interfaces. I would do what you say there. But what you're doing now is *worse*, don't you see it? You _think_ that you are not exporting any add_to_list function from scan.c because you removed the wrapper from scan.h, but then you open-code it anyway in create_vtbl (yes, that list_add _is_ the *only* meaningful code in add_to_list; it just so _happens_ that most of its callers till now also wanted to allocate a ubi_scan_leb too at that time so you pushed that too into add_to_list rather than do it in the callers -- via a separate alloc_leb, for example). Now that means there is an *undocumented* use of the "add to list" _functionality_ outside of scan.c anyway and which wouldn't even come up if someone tries to analyse / overhaul the code some day in the future. I know it's just _one_ exception, but still, I'm a heckler for style. > Read Teo's comments - I actually now agree with them. And after I had > changed UBI i got rid of several thousands lines of code, and the code > became simpler. > > So, my argument is: > 1. It makes no sense to introduce one more non-static function to _just_ > encapsulate list_add_tail and _just_ for one caller. Well, introducing one more non-static function is *not* what I originally had in mind (it was proposed only as a temporary measure till all users of ubi_scan_add_to_list could be updated to use the other version, explained below). I actually planned to _replace_ ubi_scan_add_to_list with another version that does *not* allocate memory (the __ubi_scan_add_to_list I proposed above), and leave allocation up to its *callers* who therefore pass a valid ubi_scan_leb to it. But that is where I ran into ubi_scan_add_used(). > 2. It is _C_, it is _kernel_, and it is OK sometimes _not_ to follow > computer since rules. Ah, well, forget it. It's all a matter of taste and maintainability. I guess in the end it needs to be fine with you. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ | |||||||||
| Last update: 2007-05-05 15:29 [from the cache] ©2003-2008 | ||||||||||