[haiku-gsoc] Re: Xsi semaphorses: patch #1
- From: "Salvatore Benedetto" <emitrax@xxxxxxxxx>
- To: haiku-gsoc@xxxxxxxxxxxxx
- Date: Mon, 14 Jul 2008 11:13:56 +0000
2008/7/13 Axel Dörfler <axeld@xxxxxxxxxxxxxxxx>:
> Great! I knew it was the perfect test app :-))
I managed to corrupt the partition even without bonnie++. :-)
>> + struct thread *thread;
>> [...]
>> + size_t Hash(XsiSemaphoreSet* variable) const
>
> Please choose one "* "/" *" style and stick to it consistently.
Ok.
>
>> +static OpenHashTable<SemaphoreHashTableDefinition>
>> sSemaphoreHashTable;
>> +static int sNextAvailableID = 0;
>
> First declarations, then definitions, ie. actual variables come last
> and together.
>
>> +static spinlock xsi_semaphore_set_spinlock = B_SPINLOCK_INITIALIZER;
>> +#define GRAB_SEM_LOCK() acquire_spinlock(&
>> xsi_semaphore_set_spinlock)
>> +#define RELEASE_SEM_LOCK() release_spinlock(&
>> xsi_sempahore_set_spinlock)
>
> While that copies existing code, it is a style guide violation, and
> should be fixed everywhere. First of all, the use of macros for
> something like this should be removed, and global static variables are
> called as the ones above (ie. sVariableName).
Fine by me.
>> +#define MAX_XSI_SEMAPHORE 512
>> +static int sTotalNumberOfXsiSemaphores = 0;
>> +
>> +void
>
> No tab after #define, const variables are allowed to be used, too, and
> there are two blanks between declarations and functions, too.
I've actually always used tabs after #define. Do all devs agree on this one? :-)
>
>> + ushort fNumberOfSemaphores;
>> // sem_nsems
>
> Prefer uint16 over ushort when it's what you mean; at least
> theoretically types can change their meaning with a compiler/
> architecture switch (like "long" in 64bit platforms).
I used unsigned short as it was the type defined by the standard.
> Also, less long names certainly wouldn't hurt; names should be as short
> and precise as possible, and their context dictates (ie. no reason to
> call an index variable threadTableIndex if there is only a single index
> variable involved). "fProcessesWaitingToIncrease" is also a candidate
> for a better name.
I agree with the less the better, but I've also been told not to
truncute variable
names like fNumOfSem.
>
>> +union semun {
>> + int val;
>> + struct semid_ds *buf;
>> + unsigned short *array;
>> +};
>> +
>> +int
>> +semget(key_t key, int num_sems, int sem_flags)
>
> Again order and two spaces.
> Also numSems and semFlags instead of those names (other examples follow
> here as well).
> I would also prefer to use real names like "value" and "buffer" instead
> of those above.
I thought it would be ok to use the names defined in the posix
standard as execption.
Thanks for looking into it!
Regards,
--
Salvatore Benedetto (a.k.a. emitrax)
Student of Computer Engineer
University of Pisa
www.haiku-os.it
Other related posts:
