I went through the branches Matthew had posted (thanks for doing that!) and compared the formatted code to the original using omr/hashtable/hashtable.c. I agree that 'webkit' is closest to our current standard (though I strongly prefer to use tabs rather than spaces as it helps avoid the ascii-art alignment in code). For most of the standards, the lines are very short when formatted. Can this be tweaked?
My overall impression is that the autoformatter doesn't actually get us to consistent code, it just introduces auto-generated inconsistencies. This includes unfortunate modifications like moving the '&& condition <newline> && condition <newline> && condition' to a mix of 'condition && <newline> && condition' - see examples below.
It also doesn't address the issue of missing braces for control flow (if / while / etc). A separate tool (clang-tidy?) would be needed to address these kinds of issues. If we're requiring one, do we want to require both tools?
BasedOnStyle: Google\nSortIncludes: false
==================================
The lines are unnecessary short. Most people have large monitors, no need to wrap lines.
Example 1:
-static void *hashTableAddNodeInList(J9HashTable *table, void *entry, void **head);
+static void **hashTableFindNodeSpaceOpt(J9HashTable *table, void *entry,
+ void **head);
Example 2:
+static uintptr_t comparatorToEqualFn(void *leftKey, void *rightKey,
+ void *userData) {
Loss of clarity when dealing with parameter names. Existing code can be read at a glance, the new code is much more cryptic
Example:
-collisionResilientHashTableNew(
- OMRPortLibrary *portLibrary,
- const char *tableName,
- uint32_t tableSize,
- uint32_t entrySize,
- uint32_t flags,
- uint32_t memoryCategory,
- uint32_t listToTreeThreshold,
- J9HashTableHashFn hashFn,
- J9HashTableComparatorFn comparatorFn,
- J9HashTablePrintFn printFn,
- void *functionUserData)
-{
+J9HashTable *collisionResilientHashTableNew(
+ OMRPortLibrary *portLibrary, const char *tableName, uint32_t tableSize,
+ uint32_t entrySize, uint32_t flags, uint32_t memoryCategory,
+ uint32_t listToTreeThreshold, J9HashTableHashFn hashFn,
+ J9HashTableComparatorFn comparatorFn, J9HashTablePrintFn printFn,
+ void *functionUserData) {
=====================================
Is it really necessary to split this prototype across multiple lines?
Also, the indentation means that changing the name of the method changes 3 lines of code. This kind of over-alignment of code hurts the ability to see history changes.
-static uint32_t hashTableRemoveNodeSpaceOpt(J9HashTable *table, void *entry, void **head);
+static uint32_t hashTableRemoveNodeSpaceOpt(J9HashTable* table,
+ void* entry,
+ void** head);
This might be due to the short lines but it also inconsistently applies the && at the end of the line vs the beginning of the line.
- if (J9HASH_TABLE_ALLOW_SIZE_OPTIMIZATION == ((flags & J9HASH_TABLE_ALLOW_SIZE_OPTIMIZATION))
- && (hashTable->listNodeSize == (2 * sizeof(uintptr_t)))
- && (hashTable->tableSize <= SPACE_OPT_LIMIT)
- && (0 == (flags & J9HASH_TABLE_ALLOCATE_ELEMENTS_USING_MALLOC32))
- && (!(J9HASH_TABLE_COLLISION_RESILIENT == (flags & J9HASH_TABLE_COLLISION_RESILIENT)))
- ) {
+
+ if (J9HASH_TABLE_ALLOW_SIZE_OPTIMIZATION ==
+ ((flags & J9HASH_TABLE_ALLOW_SIZE_OPTIMIZATION)) &&
+ (hashTable->listNodeSize == (2 * sizeof(uintptr_t))) &&
+ (hashTable->tableSize <= SPACE_OPT_LIMIT)
#if defined(OMR_ENV_DATA64) #if defined(OMR_ENV_DATA64)
+ && (0 == (flags & J9HASH_TABLE_ALLOCATE_ELEMENTS_USING_MALLOC32))
#endif /* OMR_ENV_DATA64 */ #endif /* OMR_ENV_DATA64 */
+ && (!(J9HASH_TABLE_COLLISION_RESILIENT ==
==================================
A lot of the same problems.
==================================
I like that it doesn't ruin the readability of the 'primesTable' array.
Handles the function arguments better. ie: hashTableNewImpl()
Apart from tabs vs spaces, I can live with this.
================================
A lot of the same problems
