|
From: | Paul Eggert |
Subject: | Re: [PATCH]: Add new bytecode op `switch' for implementing branch tables. |
Date: | Fri, 10 Feb 2017 12:47:39 -0800 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 |
On 02/10/2017 10:25 AM, Vibhav Pant wrote:
Are there any other issues before I merge this into master?
For the C code, please use the usual style: space before paren in calls, GNU style indenting for curly braces, "/* " at start of comments, main operator like "||" at start of next line rather than at end of previous line.
One of the 'if's is overparenthesized, i.e., "if ((E))" where E is an ordinary expression and "if (E)" will do.
Prefer "if (BYTE_CODE_SAFE)" to "#ifdef BYTE_CODE_SAFE", as these days it's better to avoid the preprocessor when it's easy.
This comment:/* Hash tables for switch are declared with :size set to the
exact number of cases, thus HASH_TABLE_SIZE (h) == h->count. */is something that could be checked, no? Perhaps replace the comment with "if (BYTE_CODE_SAFE) eassert (HASH_TABLE_SIZE (h) == h->count);" and do the latter even with large hash tables?
If you change the loop from "for (i = 0; i < h->count; i++)" to "for (i = h->count; 0 <= --i; )", then you can merge the two copies of "op = XINT (HASH_VALUE (h, i)); goto ob_branch;" into one copy that is after the surrounding "if".
[Prev in Thread] | Current Thread | [Next in Thread] |