| Title | Some niggles annoy DRJ |
| Status | closed |
| Priority | nice |
| Assigned user | Gareth Rees |
| Organization | Ravenbrook |
| 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[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 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.] |
| How found | inspection |
| Evidence | Looking at the code mostly [1] //info.ravenbrook.com/mail/2003/11/17/15-10-02/0.txt |
| 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. |
| 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). * 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. |