|Title||Some niggles annoy DRJ|
|Assigned user||Gareth Rees|
|Description||There's a growing pile of things that annoy DRJ. Mostly slightly untidy code and practices. Let's have a list.|
- SegSetSummary in seg.c has a very naughty #ifdef PROTECTION_NONE, as does SegSetRankAndSummary. grep for PROTECTION_NONE and #ifdef?
|Analysis||GDR 2013-03-08: It's not appropriate to fix the following:|
- what's with all the static declarations? They impede debugging. EG the backtrace that Göran mailed round didn't have the right symbol for seg.c line 751 because the function was declared static. [GDR 2013-03-08: the "static" declaration allows compilers like LLVM to aggressively inline functions.]
- seg.c has names beginning with 'GC' [GDR 2014-01-12: we can live with this.]
GDR 2012-10-26: The following items are be fixed:
- C style: inconsistent use of space after 'if' 'for' 'while' etc. We should document one style and stick to it. I prefer "if(...)" not "if (...)" [DRJ 2007-07-11: this is now documented in design/cstyle] [GDR 2014-01-12: but guide.impl.c.format.space.control says "One space between the keyword, switch, while, for and the following paren"]
- C style: inconsistent use of space in '++ variable'. Although I usually use '++variable' I quite like '++ variable' as it makes it stand out more, and that's fine for an idiom. [GDR 2014-01-12: this is guide.impl.c.format.space.op]
- fri4gc.gmk why is it i4? [GDR 2012-10-26: mistake; now fri3gc]
- fri4gc.gmk -Wpointer-arith is suppressed, but this appears to be for SuSE, is this necessary? [GDR 2012-10-26: not there any more]
- mpsi.c mps_space_create and mps_space_destroy are #ifdef MPS_PROD_DYLAN. We don't support dylan or these functions. remove them? [GDR 2012-10-26: last mention of mps_space removed from mpsi.c in change 180093.]
- lines with multiple statements in. Such as '*segReturn = seg; *rankReturn = rank' in trace.c [DRJ 2007-07-11: design/cstyle now bans this] search for ';.*;' ?
- mps_arena_park in mpsi.c takes mps_space_t, not mps_arena_t [GDR 2012-10-26: not any more]
- mps_arena_has_addr in reference manual has all sorts of silly references to mps_arena_park. [GDR 2012-10-26: not any more]
- pol should be pool in test_stepper in amcss.c, root conflicts with global. rename global.
- ArenaRootRing macro in mpm.h is not used (and can not be). ArenaTraceRing macro is not used. How do we find other broken unused macros? [GDR 2014-01-12: Fixed in change 183947.]
- Ghastly use of ternary inside "if" in amcReclaimNailed. [GDR 2014-01-12: fixed in change 183948.]
|Evidence||Looking at the code mostly|
|Created by||David Jones|
|Created on||2003-07-09 11:43:02|
|Last modified by||Gareth Rees|
|Last modified on||2014-04-10 12:26:42|
|History||2003-07-09 DRJ created|
2003-11-04 DRJ PROTECTION_NONE #ifdef
2003-11-19 DRJ too much static
2007-07-11 DRJ ternary
2012-10-26 GDR some of this has been fixed
2013-03-19 GDR Assigned to GDR.
|185176||closed||2014-04-02 15:48:57||Gareth Rees||Improve clarity of product configuration so that names more explicitly indicate what they do:
* CONFIG_POLL_NONE (because the user-visible consequence is that polling is no longer supported; was CONFIG_PROTECTION_NONE).
* DISABLE_LOCKS (was THREAD_SINGLE).
* DISABLE_SHIELD (was THREAD_SINGLE && PROTECTION_NONE)
* DISABLE_REMEMBERED_SET (was PROTECTION_NONE)
When the shield is disabled, ArenaLeave asserts that there are no busy traces, and ArenaPoll is a no-op.
By having functions implemented using the corresponding macro, we can avoid duplicated code, and avoid testing DISABLE_SHIELD in global.c.
Remove all remaining references to MPS_PROD_EPCORE.
|183948||open||2014-01-12 11:21:21||Gareth Rees||Clarify decision to preserve/reclaim in amcReclaimNailed, avoiding ternary operator inside the if condition.|
|183947||open||2014-01-12 11:19:35||Gareth Rees||Remove unused (and unusable) macros ArenaRootRing() and ArenaTraceRing().|
|181095||open||2013-03-08 12:41:41||Gareth Rees||Move global 'pool' to local so that we can rename argument 'pol' to 'pool'.|
|180093||open||2012-10-26 09:51:12||Gareth Rees||No need to include "mpsavm.h". The comment justifying it was bogus: "only for mps_space_create".|
|178941||open||2012-08-15 17:21:18||Richard Brooksby||Removing no-longer-needed warning suppression.|
|178940||open||2012-08-15 17:04:55||Richard Brooksby||Removing no-longer-required warning suppression.|
|178939||open||2012-08-15 17:01:48||Richard Brooksby||Removing no-longer-required warning supression.|
|178938||open||2012-08-15 17:00:58||Richard Brooksby||Removing no-longer-needed warning suppression.|
|178935||open||2012-08-15 15:37:42||Richard Brooksby||Correcting misnamed I4 architecture to I3. This distinction was once slightly useful for optimisation, but no longer makes any sense.|