MPS issue job000742

TitleSome niggles annoy DRJ
Assigned userGareth Rees
DescriptionThere'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?
AnalysisGDR 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[1] 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 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]

- 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.]
How foundinspection
EvidenceLooking at the code mostly
[1] //
Created byDavid Jones
Created on2003-07-09 11:43:02
Last modified byGareth Rees
Last modified on2014-04-10 12:26:42
History2003-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.


Change Effect Date User Description
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).
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.