| Message ID | 20240728141547.302478-2-alx@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | c: Add _Lengthof operator | expand |
On Sun, Jul 28, 2024 at 4:16 PM Alejandro Colomar <alx@kernel.org> wrote: > > There were two identical definitions, and none of them are available > where they are needed for implementing _Lengthof(). Merge them, and > provide the single definition in gcc/tree.{h,cc}, where it's available > for _Lengthof(). > > Signed-off-by: Alejandro Colomar <alx@kernel.org> > --- > gcc/cp/cp-tree.h | 1 - > gcc/cp/tree.cc | 13 ------------- > gcc/rust/backend/rust-tree.cc | 13 ------------- > gcc/rust/backend/rust-tree.h | 2 -- > gcc/tree.cc | 13 +++++++++++++ > gcc/tree.h | 1 + > 6 files changed, 14 insertions(+), 29 deletions(-) > > diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h > index c1a371bc721..e6c1c63f872 100644 > --- a/gcc/cp/cp-tree.h > +++ b/gcc/cp/cp-tree.h > @@ -8099,7 +8099,6 @@ extern tree build_exception_variant (tree, tree); > extern void fixup_deferred_exception_variants (tree, tree); > extern tree bind_template_template_parm (tree, tree); > extern tree array_type_nelts_total (tree); > -extern tree array_type_nelts_top (tree); > extern bool array_of_unknown_bound_p (const_tree); > extern tree break_out_target_exprs (tree, bool = false); > extern tree build_ctor_subob_ref (tree, tree, tree); > diff --git a/gcc/cp/tree.cc b/gcc/cp/tree.cc > index dfd4a3a948b..1f3ecff1a21 100644 > --- a/gcc/cp/tree.cc > +++ b/gcc/cp/tree.cc > @@ -3071,19 +3071,6 @@ cxx_print_statistics (void) > depth_reached); > } > > -/* Return, as an INTEGER_CST node, the number of elements for TYPE > - (which is an ARRAY_TYPE). This counts only elements of the top > - array. */ > - > -tree > -array_type_nelts_top (tree type) > -{ > - return fold_build2_loc (input_location, > - PLUS_EXPR, sizetype, > - array_type_nelts (type), > - size_one_node); > -} > - > /* Return, as an INTEGER_CST node, the number of elements for TYPE > (which is an ARRAY_TYPE). This one is a recursive count of all > ARRAY_TYPEs that are clumped together. */ > diff --git a/gcc/rust/backend/rust-tree.cc b/gcc/rust/backend/rust-tree.cc > index 2a5ffcbf895..dd8eda84f9b 100644 > --- a/gcc/rust/backend/rust-tree.cc > +++ b/gcc/rust/backend/rust-tree.cc > @@ -859,19 +859,6 @@ is_empty_class (tree type) > return CLASSTYPE_EMPTY_P (type); > } > > -// forked from gcc/cp/tree.cc array_type_nelts_top > - > -/* Return, as an INTEGER_CST node, the number of elements for TYPE > - (which is an ARRAY_TYPE). This counts only elements of the top > - array. */ > - > -tree > -array_type_nelts_top (tree type) > -{ > - return fold_build2_loc (input_location, PLUS_EXPR, sizetype, > - array_type_nelts (type), size_one_node); > -} > - > // forked from gcc/cp/tree.cc builtin_valid_in_constant_expr_p > > /* Test whether DECL is a builtin that may appear in a > diff --git a/gcc/rust/backend/rust-tree.h b/gcc/rust/backend/rust-tree.h > index 26c8b653ac6..e597c3ab81d 100644 > --- a/gcc/rust/backend/rust-tree.h > +++ b/gcc/rust/backend/rust-tree.h > @@ -2993,8 +2993,6 @@ extern location_t rs_expr_location (const_tree); > extern int > is_empty_class (tree type); > > -extern tree array_type_nelts_top (tree); > - > extern bool > is_really_empty_class (tree, bool); > > diff --git a/gcc/tree.cc b/gcc/tree.cc > index 2d2d5b6db6e..3b0adb4cd9f 100644 > --- a/gcc/tree.cc > +++ b/gcc/tree.cc > @@ -3729,6 +3729,19 @@ array_type_nelts (const_tree type) > ? max > : fold_build2 (MINUS_EXPR, TREE_TYPE (max), max, min)); > } > + > +/* Return, as an INTEGER_CST node, the number of elements for TYPE > + (which is an ARRAY_TYPE). This counts only elements of the top > + array. */ > + > +tree > +array_type_nelts_top (tree type) > +{ > + return fold_build2_loc (input_location, > + PLUS_EXPR, sizetype, > + array_type_nelts (type), > + size_one_node); > +} But this is now extremely confusing API with array_type_nelts above this saying /* Return, as a tree node, the number of elements for TYPE (which is an ARRAY_TYPE) minus one. This counts only elements of the top array. */ so both are "_top". And there's build_array_type_nelts that's taking the number of elements. Can you please rename the existing array_type_nelts to array_type_nelts_minus_one? Then _top could be dropped as well from the alternate API you add. I'll also note since array_type_nelts_top calls the other function and that has /* If they did it with unspecified bounds, then we should have already given an error about it before we got here. */ if (! TYPE_DOMAIN (type)) return error_mark_node; the function should handle error_mark_node (and pass that down). Note array_type_nelts returns nelts - 1 because that avoids building a new tree node for arrays with lower bound zero. Thanks, Richard. > /* If arg is static -- a reference to an object in static storage -- then > return the object. This is not the same as the C meaning of `static'. > diff --git a/gcc/tree.h b/gcc/tree.h > index 28e8e71b036..c620e55b68d 100644 > --- a/gcc/tree.h > +++ b/gcc/tree.h > @@ -4922,6 +4922,7 @@ extern tree build_method_type (tree, tree); > extern tree build_offset_type (tree, tree); > extern tree build_complex_type (tree, bool named = false); > extern tree array_type_nelts (const_tree); > +extern tree array_type_nelts_top (tree); > > extern tree value_member (tree, tree); > extern tree purpose_member (const_tree, tree); > -- > 2.45.2 >
Hi Richard, On Mon, Jul 29, 2024 at 10:27:35AM GMT, Richard Biener wrote: > On Sun, Jul 28, 2024 at 4:16 PM Alejandro Colomar <alx@kernel.org> wrote: > > > > There were two identical definitions, and none of them are available > > where they are needed for implementing _Lengthof(). Merge them, and > > provide the single definition in gcc/tree.{h,cc}, where it's available > > for _Lengthof(). > > > > Signed-off-by: Alejandro Colomar <alx@kernel.org> > > --- > > gcc/cp/cp-tree.h | 1 - > > gcc/cp/tree.cc | 13 ------------- > > gcc/rust/backend/rust-tree.cc | 13 ------------- > > gcc/rust/backend/rust-tree.h | 2 -- > > gcc/tree.cc | 13 +++++++++++++ > > gcc/tree.h | 1 + > > 6 files changed, 14 insertions(+), 29 deletions(-) > > [...] > > diff --git a/gcc/tree.cc b/gcc/tree.cc > > index 2d2d5b6db6e..3b0adb4cd9f 100644 > > --- a/gcc/tree.cc > > +++ b/gcc/tree.cc > > @@ -3729,6 +3729,19 @@ array_type_nelts (const_tree type) > > ? max > > : fold_build2 (MINUS_EXPR, TREE_TYPE (max), max, min)); > > } > > + > > +/* Return, as an INTEGER_CST node, the number of elements for TYPE > > + (which is an ARRAY_TYPE). This counts only elements of the top > > + array. */ > > + > > +tree > > +array_type_nelts_top (tree type) > > +{ > > + return fold_build2_loc (input_location, > > + PLUS_EXPR, sizetype, > > + array_type_nelts (type), > > + size_one_node); > > +} > > But this is now extremely confusing API with array_type_nelts above this > saying > > /* Return, as a tree node, the number of elements for TYPE (which is an > ARRAY_TYPE) minus one. This counts only elements of the top array. */ > > so both are "_top". And there's build_array_type_nelts that's taking > the number of elements. > > Can you please rename the existing array_type_nelts to > array_type_nelts_minus_one? Then _top could be dropped as well from > the alternate API you add. I wanted to do that, but then I found other functions that are named similarly, such as build_array_type_nelts(), and thought that I wasn't sure if all of them should be renamed to _minus_one, or just some. So I decided to start without renaming. But yeah, I think I should rename. I'll prepare a patch for renaming it independently of this patch set, and send it to be merged before this patch set. > I'll also note since array_type_nelts_top calls the other function and that has > > /* If they did it with unspecified bounds, then we should have already > given an error about it before we got here. */ > if (! TYPE_DOMAIN (type)) > return error_mark_node; > > the function should handle error_mark_node (and pass that down). Hmmmm, now I understand that (! TYPE_DOMAIN (type)) $ grep -rn return.array_type_nelts gcc gcc/cp/call.cc:12111: return array_type_nelts_top (c->type); gcc/c-family/c-common.cc:4090: return array_type_nelts_top (type); $ sed -n 12102,12119p gcc/cp/call.cc /* Return a tree representing the number of elements initialized by the list-initialization C. The caller must check that C converts to an array type. */ static tree nelts_initialized_by_list_init (conversion *c) { /* If the array we're converting to has a dimension, we'll use that. */ if (TYPE_DOMAIN (c->type)) return array_type_nelts_top (c->type); else { /* Otherwise, we look at how many elements the constructor we're initializing from has. */ tree ctor = conv_get_original_expr (c); return size_int (CONSTRUCTOR_NELTS (ctor)); } } It seems that would fail when measuring for example #define memberof(T, member) ((T){}.member) struct s { int x; int a[]; }; __lengthof__(memberof(struct s, a)); I guess? $ cat len.c #include <stdio.h> #define memberof(T, member) ((T){}.member) struct s { int x; int y[]; }; int main(int argc, char *argv[argc + 1]) { int a[42]; size_t n; (void) argv; //n = __lengthof__(argv); //printf("__lengthof__(argv) == %zu\n", n); n = __lengthof__(a); printf("lengthof(a):\t %zu\n", n); n = __lengthof__(long [99]); printf("lengthof(long [99]):\t %zu\n", n); n = __lengthof__(short [n - 10]); printf("lengthof(short [n - 10]):\t %zu\n", n); int b[n / 2]; n = __lengthof__(b); printf("lengthof(b):\t %zu\n", n); n = __lengthof__(memberof(struct s, y)); printf("lengthof(memberof(struct s, y)):\t %zu\n", n); } alx@debian:~/tmp/gcc$ /opt/local/gnu/gcc/lengthof/bin/gcc len.c alx@debian:~/tmp/gcc$ ./a.out lengthof(a): 42 lengthof(long [99]): 99 lengthof(short [n - 10]): 89 lengthof(b): 44 lengthof(memberof(struct s, y)): 44 Indeed, it's misbehaving. I'll have a look at that. I'll probably have to add an error similar to sizeof's one: len.c: In function ‘main’: len.c:37:19: error: invalid application of ‘sizeof’ to incomplete type ‘int[]’ 37 | n = sizeof(memberof(struct s, y)); | ^ Thanks! > > Note array_type_nelts returns nelts - 1 because that avoids building > a new tree node for arrays with lower bound zero. What does it mean that the lower bound is zero? I didn't understand that part. > > Thanks, > Richard. > > > /* If arg is static -- a reference to an object in static storage -- then > > return the object. This is not the same as the C meaning of `static'. Have a lovely day! Alex
On Mon, Jul 29, 2024 at 10:55 AM Alejandro Colomar <alx@kernel.org> wrote: > > Hi Richard, > > On Mon, Jul 29, 2024 at 10:27:35AM GMT, Richard Biener wrote: > > On Sun, Jul 28, 2024 at 4:16 PM Alejandro Colomar <alx@kernel.org> wrote: > > > > > > There were two identical definitions, and none of them are available > > > where they are needed for implementing _Lengthof(). Merge them, and > > > provide the single definition in gcc/tree.{h,cc}, where it's available > > > for _Lengthof(). > > > > > > Signed-off-by: Alejandro Colomar <alx@kernel.org> > > > --- > > > gcc/cp/cp-tree.h | 1 - > > > gcc/cp/tree.cc | 13 ------------- > > > gcc/rust/backend/rust-tree.cc | 13 ------------- > > > gcc/rust/backend/rust-tree.h | 2 -- > > > gcc/tree.cc | 13 +++++++++++++ > > > gcc/tree.h | 1 + > > > 6 files changed, 14 insertions(+), 29 deletions(-) > > > > > [...] > > > > diff --git a/gcc/tree.cc b/gcc/tree.cc > > > index 2d2d5b6db6e..3b0adb4cd9f 100644 > > > --- a/gcc/tree.cc > > > +++ b/gcc/tree.cc > > > @@ -3729,6 +3729,19 @@ array_type_nelts (const_tree type) > > > ? max > > > : fold_build2 (MINUS_EXPR, TREE_TYPE (max), max, min)); > > > } > > > + > > > +/* Return, as an INTEGER_CST node, the number of elements for TYPE > > > + (which is an ARRAY_TYPE). This counts only elements of the top > > > + array. */ > > > + > > > +tree > > > +array_type_nelts_top (tree type) > > > +{ > > > + return fold_build2_loc (input_location, > > > + PLUS_EXPR, sizetype, > > > + array_type_nelts (type), > > > + size_one_node); > > > +} > > > > But this is now extremely confusing API with array_type_nelts above this > > saying > > > > /* Return, as a tree node, the number of elements for TYPE (which is an > > ARRAY_TYPE) minus one. This counts only elements of the top array. */ > > > > so both are "_top". And there's build_array_type_nelts that's taking > > the number of elements. > > > > Can you please rename the existing array_type_nelts to > > array_type_nelts_minus_one? Then _top could be dropped as well from > > the alternate API you add. > > I wanted to do that, but then I found other functions that are named > similarly, such as build_array_type_nelts(), and thought that I wasn't > sure if all of them should be renamed to _minus_one, or just some. So > I decided to start without renaming. Just array_type_nelts needs renaming, build_array_type_nelts is fine. > But yeah, I think I should rename. I'll prepare a patch for renaming it > independently of this patch set, and send it to be merged before this > patch set. Thanks. > > I'll also note since array_type_nelts_top calls the other function and that has > > > > /* If they did it with unspecified bounds, then we should have already > > given an error about it before we got here. */ > > if (! TYPE_DOMAIN (type)) > > return error_mark_node; > > > > the function should handle error_mark_node (and pass that down). > > Hmmmm, now I understand that (! TYPE_DOMAIN (type)) > > $ grep -rn return.array_type_nelts gcc > gcc/cp/call.cc:12111: return array_type_nelts_top (c->type); > gcc/c-family/c-common.cc:4090: return array_type_nelts_top (type); > > $ sed -n 12102,12119p gcc/cp/call.cc > /* Return a tree representing the number of elements initialized by the > list-initialization C. The caller must check that C converts to an > array type. */ > > static tree > nelts_initialized_by_list_init (conversion *c) > { > /* If the array we're converting to has a dimension, we'll use that. */ > if (TYPE_DOMAIN (c->type)) > return array_type_nelts_top (c->type); > else > { > /* Otherwise, we look at how many elements the constructor we're > initializing from has. */ > tree ctor = conv_get_original_expr (c); > return size_int (CONSTRUCTOR_NELTS (ctor)); > } > } The point is that if you make this a general API it should be safe to be used, not depending on constraints that are apparently checked right now. > It seems that would fail when measuring for example > > #define memberof(T, member) ((T){}.member) > > struct s { > int x; > int a[]; > }; > > __lengthof__(memberof(struct s, a)); > > I guess? > > $ cat len.c > #include <stdio.h> > > #define memberof(T, member) ((T){}.member) > > struct s { > int x; > int y[]; > }; > > int > main(int argc, char *argv[argc + 1]) > { > int a[42]; > size_t n; > > (void) argv; > > //n = __lengthof__(argv); > //printf("__lengthof__(argv) == %zu\n", n); > > n = __lengthof__(a); > printf("lengthof(a):\t %zu\n", n); > > n = __lengthof__(long [99]); > printf("lengthof(long [99]):\t %zu\n", n); > > n = __lengthof__(short [n - 10]); > printf("lengthof(short [n - 10]):\t %zu\n", n); > > int b[n / 2]; > n = __lengthof__(b); > printf("lengthof(b):\t %zu\n", n); > > n = __lengthof__(memberof(struct s, y)); > printf("lengthof(memberof(struct s, y)):\t %zu\n", n); > } > alx@debian:~/tmp/gcc$ /opt/local/gnu/gcc/lengthof/bin/gcc len.c > alx@debian:~/tmp/gcc$ ./a.out > lengthof(a): 42 > lengthof(long [99]): 99 > lengthof(short [n - 10]): 89 > lengthof(b): 44 > lengthof(memberof(struct s, y)): 44 > > Indeed, it's misbehaving. I'll have a look at that. I'll probably have > to add an error similar to sizeof's one: > > len.c: In function ‘main’: > len.c:37:19: error: invalid application of ‘sizeof’ to incomplete type ‘int[]’ > 37 | n = sizeof(memberof(struct s, y)); > | ^ > > Thanks! > > > > > Note array_type_nelts returns nelts - 1 because that avoids building > > a new tree node for arrays with lower bound zero. > > What does it mean that the lower bound is zero? I didn't understand > that part. It means the function can return TYPE_MAX_VALUE of the TYPE_DOMAIN unchanged. Richard. > > > > Thanks, > > Richard. > > > > > /* If arg is static -- a reference to an object in static storage -- then > > > return the object. This is not the same as the C meaning of `static'. > > Have a lovely day! > Alex > > -- > <https://www.alejandro-colomar.es/>
diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h index c1a371bc721..e6c1c63f872 100644 --- a/gcc/cp/cp-tree.h +++ b/gcc/cp/cp-tree.h @@ -8099,7 +8099,6 @@ extern tree build_exception_variant (tree, tree); extern void fixup_deferred_exception_variants (tree, tree); extern tree bind_template_template_parm (tree, tree); extern tree array_type_nelts_total (tree); -extern tree array_type_nelts_top (tree); extern bool array_of_unknown_bound_p (const_tree); extern tree break_out_target_exprs (tree, bool = false); extern tree build_ctor_subob_ref (tree, tree, tree); diff --git a/gcc/cp/tree.cc b/gcc/cp/tree.cc index dfd4a3a948b..1f3ecff1a21 100644 --- a/gcc/cp/tree.cc +++ b/gcc/cp/tree.cc @@ -3071,19 +3071,6 @@ cxx_print_statistics (void) depth_reached); } -/* Return, as an INTEGER_CST node, the number of elements for TYPE - (which is an ARRAY_TYPE). This counts only elements of the top - array. */ - -tree -array_type_nelts_top (tree type) -{ - return fold_build2_loc (input_location, - PLUS_EXPR, sizetype, - array_type_nelts (type), - size_one_node); -} - /* Return, as an INTEGER_CST node, the number of elements for TYPE (which is an ARRAY_TYPE). This one is a recursive count of all ARRAY_TYPEs that are clumped together. */ diff --git a/gcc/rust/backend/rust-tree.cc b/gcc/rust/backend/rust-tree.cc index 2a5ffcbf895..dd8eda84f9b 100644 --- a/gcc/rust/backend/rust-tree.cc +++ b/gcc/rust/backend/rust-tree.cc @@ -859,19 +859,6 @@ is_empty_class (tree type) return CLASSTYPE_EMPTY_P (type); } -// forked from gcc/cp/tree.cc array_type_nelts_top - -/* Return, as an INTEGER_CST node, the number of elements for TYPE - (which is an ARRAY_TYPE). This counts only elements of the top - array. */ - -tree -array_type_nelts_top (tree type) -{ - return fold_build2_loc (input_location, PLUS_EXPR, sizetype, - array_type_nelts (type), size_one_node); -} - // forked from gcc/cp/tree.cc builtin_valid_in_constant_expr_p /* Test whether DECL is a builtin that may appear in a diff --git a/gcc/rust/backend/rust-tree.h b/gcc/rust/backend/rust-tree.h index 26c8b653ac6..e597c3ab81d 100644 --- a/gcc/rust/backend/rust-tree.h +++ b/gcc/rust/backend/rust-tree.h @@ -2993,8 +2993,6 @@ extern location_t rs_expr_location (const_tree); extern int is_empty_class (tree type); -extern tree array_type_nelts_top (tree); - extern bool is_really_empty_class (tree, bool); diff --git a/gcc/tree.cc b/gcc/tree.cc index 2d2d5b6db6e..3b0adb4cd9f 100644 --- a/gcc/tree.cc +++ b/gcc/tree.cc @@ -3729,6 +3729,19 @@ array_type_nelts (const_tree type) ? max : fold_build2 (MINUS_EXPR, TREE_TYPE (max), max, min)); } + +/* Return, as an INTEGER_CST node, the number of elements for TYPE + (which is an ARRAY_TYPE). This counts only elements of the top + array. */ + +tree +array_type_nelts_top (tree type) +{ + return fold_build2_loc (input_location, + PLUS_EXPR, sizetype, + array_type_nelts (type), + size_one_node); +} /* If arg is static -- a reference to an object in static storage -- then return the object. This is not the same as the C meaning of `static'. diff --git a/gcc/tree.h b/gcc/tree.h index 28e8e71b036..c620e55b68d 100644 --- a/gcc/tree.h +++ b/gcc/tree.h @@ -4922,6 +4922,7 @@ extern tree build_method_type (tree, tree); extern tree build_offset_type (tree, tree); extern tree build_complex_type (tree, bool named = false); extern tree array_type_nelts (const_tree); +extern tree array_type_nelts_top (tree); extern tree value_member (tree, tree); extern tree purpose_member (const_tree, tree);
There were two identical definitions, and none of them are available where they are needed for implementing _Lengthof(). Merge them, and provide the single definition in gcc/tree.{h,cc}, where it's available for _Lengthof(). Signed-off-by: Alejandro Colomar <alx@kernel.org> --- gcc/cp/cp-tree.h | 1 - gcc/cp/tree.cc | 13 ------------- gcc/rust/backend/rust-tree.cc | 13 ------------- gcc/rust/backend/rust-tree.h | 2 -- gcc/tree.cc | 13 +++++++++++++ gcc/tree.h | 1 + 6 files changed, 14 insertions(+), 29 deletions(-)