Source-Changes-HG archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

[src/trunk]: src/usr.bin/xlint/lint1 lint: add comments about things left to ...



details:   https://anonhg.NetBSD.org/src/rev/bfb9a8cb6008
branches:  trunk
changeset: 1019924:bfb9a8cb6008
user:      rillig <rillig%NetBSD.org@localhost>
date:      Thu Mar 25 13:10:19 2021 +0000

description:
lint: add comments about things left to do, from code review

No functional change.

diffstat:

 usr.bin/xlint/lint1/init.c |  186 +++++++++++++++++++++++++++++++++++++-------
 1 files changed, 157 insertions(+), 29 deletions(-)

diffs (truncated from 520 to 300 lines):

diff -r b64c8881b5b0 -r bfb9a8cb6008 usr.bin/xlint/lint1/init.c
--- a/usr.bin/xlint/lint1/init.c        Thu Mar 25 10:03:26 2021 +0000
+++ b/usr.bin/xlint/lint1/init.c        Thu Mar 25 13:10:19 2021 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: init.c,v 1.118 2021/03/25 01:42:53 rillig Exp $        */
+/*     $NetBSD: init.c,v 1.119 2021/03/25 13:10:19 rillig Exp $        */
 
 /*
  * Copyright (c) 1994, 1995 Jochen Pohl
@@ -37,7 +37,7 @@
 
 #include <sys/cdefs.h>
 #if defined(__RCSID) && !defined(lint)
-__RCSID("$NetBSD: init.c,v 1.118 2021/03/25 01:42:53 rillig Exp $");
+__RCSID("$NetBSD: init.c,v 1.119 2021/03/25 13:10:19 rillig Exp $");
 #endif
 
 #include <stdlib.h>
@@ -59,21 +59,30 @@
  *     int array_nested[2][2] = { { 11, 12 }, { 21, 22 } };
  *
  *     struct { int x, y; } point = { 3, 4 };
- *     struct { int x, y; } point = { .y = 3, .x = 4 };
+ *     struct { int x, y; } point = { .y = 4, .x = 3 };
  *
- * Any scalar expression in the initializer may be surrounded by an extra pair
- * of braces, like in the example 'number_with_braces' (C99 6.7.8p11).  For
- * multi-dimensional arrays, the inner braces may be omitted like in
+ * Any scalar expression in the initializer may be surrounded by arbitrarily
+ * many extra pairs of braces, like in the example 'number_with_braces' (C99
+ * 6.7.8p11).
+ *
+ * For multi-dimensional arrays, the inner braces may be omitted like in
  * array_flat or spelled out like in array_nested.
  *
  * For the initializer, the grammar parser calls these functions:
  *
- *     init_lbrace     for each '{'
- *     init_using_expr for each value
- *     init_rbrace     for each '}'
+ *     begin_initialization
+ *             init_lbrace                     for each '{'
+ *             designator_push_name            for each '.member' before '='
+ *             designator_push_subscript       for each '[123]' before '='
+ *             init_using_expr                 for each expression
+ *             init_rbrace                     for each '}'
+ *     end_initialization
  *
  * The state of the current initialization is stored in initstk, a stack of
  * initstack_element, one element per type aggregate level.
+ * XXX: Or is that "one element per brace level"?  C99 mandates in 6.7.8p17
+ * that "each brace-enclosed initializer list has an associated current
+ * object".
  *
  * Most of the time, the topmost level of initstk contains a scalar type, and
  * its remaining count toggles between 1 and 0.
@@ -87,16 +96,11 @@
 /*
  * Type of stack which is used for initialization of aggregate types.
  *
- * XXX: Since C99, a stack is an inappropriate data structure for modelling
- * an initialization, since the designators don't have to be listed in a
- * particular order and can designate parts of sub-objects.  The member names
+ * XXX: Since C99, the initializers can be listed in arbitrary order by using
+ * designators to specify the sub-object to be initialized.  The member names
  * of non-leaf structs may thus appear repeatedly, as demonstrated in
  * d_init_pop_member.c.
  *
- * XXX: During initialization, there may be members of the top-level struct
- * that are partially initialized.  The simple i_remaining cannot model this
- * appropriately.
- *
  * See C99 6.7.8, which spans 6 pages full of tricky details and carefully
  * selected examples.
  */
@@ -107,8 +111,11 @@
         *
         * On the outermost element, this is always NULL since the outermost
         * initializer-expression may be enclosed in an optional pair of
-        * braces.  This optional pair of braces is handled by the combination
-        * of i_type and i_subt.
+        * braces, as of the current implementation.
+        *
+        * FIXME: This approach is wrong.  It's not that the outermost
+        * initializer may be enclosed in additional braces, it's every scalar
+        * that may be enclosed in additional braces, as of C99 6.7.8p11.
         *
         * Everywhere else it is nonnull.
         */
@@ -131,28 +138,48 @@
         *
         * During initialization, only the top 2 elements of the stack are
         * looked at.
+        *
+        * XXX: Having i_subt here is the wrong approach, it should not be
+        * necessary at all; see i_type.
         */
        type_t  *i_subt;
 
        /*
-        * This level of the initializer requires a '}' to be completed.
+        * Whether this level of the initializer requires a '}' to be
+        * completed.
         *
         * Multidimensional arrays do not need a closing brace to complete
         * an inner array; for example, { 1, 2, 3, 4 } is a valid initializer
-        * for int arr[2][2].
+        * for 'int arr[2][2]'.
         *
         * TODO: Do structs containing structs need a closing brace?
         * TODO: Do arrays of structs need a closing brace after each struct?
+        *
+        * XXX: Double-check whether this is the correct approach at all; see
+        * i_type.
         */
        bool i_brace: 1;
 
        /* Whether i_type is an array of unknown size. */
        bool i_array_of_unknown_size: 1;
+
+       /*
+        * XXX: This feels wrong.  Whether or not there has been a named
+        * initializer (called 'designation' since C99) should not matter at
+        * all.  Even after an initializer with designation, counting of the
+        * remaining elements continues, see C99 6.7.8p17.
+        */
        bool i_seen_named_member: 1;
 
        /*
         * For structs, the next member to be initialized by an initializer
         * without an optional designator.
+        *
+        * FIXME: The name is wrong.  C99 defines the "current object" as
+        * being the subobject being initialized, while this is rather the
+        * next member.  This only makes sense for structs anyway and should
+        * be amended by i_next_subscript for arrays.  See C99 6.7.8p17 and
+        * footnote 128 for unions.
         */
        sym_t *i_current_object;
 
@@ -170,14 +197,23 @@
         * XXX: for structs?
         * XXX: for unions?
         * XXX: for arrays?
+        *
+        * XXX: Having the count of remaining objects should not be necessary.
+        * It is probably clearer to use i_next_member and i_next_subscript
+        * (as suggested in i_current_object) for this purpose.
         */
        int i_remaining;
 
        /*
         * The initialization state of the enclosing data structure
         * (struct, union, array).
+        *
+        * XXX: Or for a scalar, for the top-level element, or for expressions
+        * in redundant braces such as '{{{{ 0 }}}}' (not yet implemented as
+        * of 2021-03-25).
         */
        struct initstack_element *i_enclosing;
+
 } initstack_element;
 
 /*
@@ -223,8 +259,12 @@
 static struct initialization *init;
 
 
+/* XXX: unnecessary prototype since it is not recursive */
 static bool    init_array_using_string(tnode_t *);
 
+
+/* TODO: replace the following functions with current_init */
+
 bool *
 current_initerr(void)
 {
@@ -253,10 +293,10 @@
        return &init->initstk;
 }
 
-#define initerr (*current_initerr())
-#define initsym (*current_initsym())
-#define initstk (*current_initstk())
-#define namedmem (*current_namedmem())
+#define initerr                (*current_initerr())
+#define initsym                (*current_initsym())
+#define initstk                (*current_initstk())
+#define namedmem       (*current_namedmem())
 
 #ifndef DEBUG
 
@@ -330,9 +370,18 @@
        debug_printf("\n");
 }
 
+/*
+ * TODO: try whether a single-line output is more readable
+ *
+ * TODO: only log the top of the stack after each modifying operation
+ *
+ * TODO: wrap all write accesses to initstack_element in setter functions
+ */
 static void
 debug_initstack_element(const initstack_element *elem)
 {
+       /* TODO: use debug_ind++ instead of the leading spaces here */
+
        if (elem->i_type != NULL)
                debug_step("  i_type           = %s", type_name(elem->i_type));
        if (elem->i_subt != NULL)
@@ -354,6 +403,9 @@
        debug_step("  i_remaining      = %d", elem->i_remaining);
 }
 
+/*
+ * TODO: only call debug_initstack after each push/pop.
+ */
 static void
 debug_initstack(void)
 {
@@ -410,7 +462,7 @@
                /*
                 * XXX: Why is this a circular list?
                 * XXX: Why is this a doubly-linked list?
-                * A simple stack should suffice.
+                * A simple queue should suffice.
                 */
                nam->n_prev = nam->n_next = nam;
                namedmem = nam;
@@ -425,11 +477,21 @@
 }
 
 /*
- * A struct member that has array type is initialized using a designator.
+ * A sub-object of an array is initialized using a designator.  This does not
+ * have to be an array element directly, it can also be used to initialize
+ * only a sub-object of the array element.
  *
  * C99 example: struct { int member[4]; } var = { [2] = 12345 };
  *
  * GNU example: struct { int member[4]; } var = { [1 ... 3] = 12345 };
+ *
+ * TODO: test the following initialization with an outer and an inner type:
+ *
+ * .deeply[0].nested = {
+ *     .deeply[1].nested = {
+ *             12345,
+ *     },
+ * }
  */
 void
 designator_push_subscript(range_t range)
@@ -440,6 +502,7 @@
        debug_leave();
 }
 
+/* TODO: add support for array subscripts, not only named members */
 static void
 designator_shift_name(void)
 {
@@ -460,6 +523,8 @@
 /*
  * Initialize the initialization stack by putting an entry for the object
  * which is to be initialized on it.
+ *
+ * TODO: merge into begin_initialization
  */
 void
 initstack_init(void)
@@ -469,11 +534,14 @@
        if (initerr)
                return;
 
+       /* TODO: merge into end_initialization */
        /* free memory used in last initialization */
        while ((istk = initstk) != NULL) {
                initstk = istk->i_enclosing;
                free(istk);
        }
+
+       /* TODO: merge into init_using_expr */
        while (namedmem != NULL)
                designator_shift_name();
 
@@ -485,6 +553,7 @@
         */
        if (initsym->s_type->t_tspec == ARRAY && is_incomplete(initsym->s_type))
                initsym->s_type = duptyp(initsym->s_type);
+       /* TODO: does 'duptyp' create a memory leak? */
 
        istk = initstk = xcalloc(1, sizeof (initstack_element));
        istk->i_subt = initsym->s_type;
@@ -494,12 +563,17 @@
        debug_leave();
 }
 
+/* TODO: document me */
 static void
 initstack_pop_item_named_member(void)



Home | Main Index | Thread Index | Old Index