At 03/01/2012 04:33 PM, HATAYAMA Daisuke Wrote: > From: Wen Congyang <we...@cn.fujitsu.com> > Subject: [RFC][PATCH 01/14 v7] Add API to create memory mapping list > Date: Thu, 01 Mar 2012 10:39:35 +0800 > >> +static void memory_mapping_list_add_mapping_sorted(MemoryMappingList *list, >> + MemoryMapping *mapping) > >> +void memory_mapping_list_add_sorted(MemoryMappingList *list, > > These functions not only sort but also merge elements of mapping > list. Is there another name that represents what these are doing more > properly? > >> + target_phys_addr_t phys_addr, >> + target_phys_addr_t virt_addr, >> + ram_addr_t length) >> +{ >> + MemoryMapping *memory_mapping, *last_mapping; >> + >> + if (QTAILQ_EMPTY(&list->head)) { >> + create_new_memory_mapping(list, phys_addr, virt_addr, length); >> + return; >> + } >> + >> + last_mapping = list->last_mapping; >> + if (last_mapping) { >> + if ((phys_addr == (last_mapping->phys_addr + last_mapping->length)) >> && >> + (virt_addr == (last_mapping->virt_addr + >> last_mapping->length))) { >> + last_mapping->length += length; >> + return; >> + } >> + } >> + >> + QTAILQ_FOREACH(memory_mapping, &list->head, next) { >> + last_mapping = memory_mapping; >> + if ((phys_addr == (last_mapping->phys_addr + last_mapping->length)) >> && >> + (virt_addr == (last_mapping->virt_addr + >> last_mapping->length))) { >> + last_mapping->length += length; >> + list->last_mapping = last_mapping; >> + return; >> + } > > Please don't use a single variable in multiple meanings in the same > function. It appears to me that the variable last_mapping is used as > n-th entry connected to the list->head within this for loop. So > this_mapping, for example, is reasonable rather than last_mapping. All > use of last_mapping within the for loop can be replaced with > memory_mapping, right?
OK > >> + >> + if (phys_addr + length < last_mapping->phys_addr) { >> + /* create a new region before last_mapping */ >> + break; >> + } >> + >> + if (phys_addr >= (last_mapping->phys_addr + last_mapping->length)) { >> + /* last_mapping does not contain this region */ >> + continue; >> + } >> + >> + if ((virt_addr - last_mapping->virt_addr) != >> + (phys_addr - last_mapping->phys_addr)) { >> + /* >> + * last_mapping contains this region, but we cannot merge this >> + * region into last_mapping. Try the next memory mapping. >> + */ >> + continue; >> + } > > Does this condition means the current mapping and the last mapping are > contiguous both phisically and viurtually? If so, it's better to write > the condition so readers can understand that more easily. current mapping and last mapping are always contiguous both phisically and virtually. > >> + >> + /* merge this region into last_mapping */ >> + if (virt_addr < last_mapping->virt_addr) { >> + last_mapping->length += last_mapping->virt_addr - virt_addr; >> + last_mapping->virt_addr = virt_addr; >> + } >> + >> + if ((virt_addr + length) > >> + (last_mapping->virt_addr + last_mapping->length)) { >> + last_mapping->length = virt_addr + length - >> last_mapping->virt_addr; >> + } >> + >> + list->last_mapping = last_mapping; >> + return; >> + } >> + >> + /* this region can not be merged into any existed memory mapping. */ >> + create_new_memory_mapping(list, phys_addr, virt_addr, length); >> +} > > How about adding helper functions for expressing each conditionals? > Just like below. Then I think the code gets more readable. > > bool mapping_proper_succeor(MemoryMapping *map, > target_phys_addr_t phys_addr, > target_virt_addr_t virt_addr); > bool mapping_physically_contains(MemoryMapping *map, > target_phys_addr_t phys_addr); > bool mapping_physically_virtually_contiguous(MemoryMapping *map, > target_phys_addr_t phys_addr, > target_virt_addr_t virt_addr); > void mapping_merge(MemoryMapping *map, target_phys_addr_t phys_addr, > target_virt_addr_t virt_addr); > > I'm not confident of the naming, these are example, and assuming > define all as static inline functions. Hmm, I think this inline functions make the code more readable. So I will modify my code. Thanks Wen Congyang > > Thanks. > HATAYAMA, Daisuke > >