Re: [PATCH v3] c, c++: Implement -Wsizeof-array-div [PR91741]
The C parts are OK. -- Joseph S. Myers jos...@codesourcery.com
Re: [PATCH v3] c, c++: Implement -Wsizeof-array-div [PR91741]
Ping.
On Thu, Oct 08, 2020 at 11:04:40AM -0400, Marek Polacek via Gcc-patches wrote:
> Ping for the C parts.
>
> On Mon, Sep 28, 2020 at 02:15:41PM -0400, Marek Polacek via Gcc-patches wrote:
> > On Tue, Sep 22, 2020 at 04:07:41PM -0400, Jason Merrill wrote:
> > > On 9/22/20 1:29 PM, Marek Polacek wrote:
> > > > Ping.
> > >
> > > The C++ change is OK.
> >
> > Ping for the C parts.
> >
> > > > On Tue, Sep 15, 2020 at 04:33:05PM -0400, Marek Polacek via Gcc-patches
> > > > wrote:
> > > > > On Tue, Sep 15, 2020 at 09:04:41AM +0200, Jakub Jelinek via
> > > > > Gcc-patches wrote:
> > > > > > On Mon, Sep 14, 2020 at 09:30:44PM -0400, Marek Polacek via
> > > > > > Gcc-patches wrote:
> > > > > > > --- a/gcc/c/c-tree.h
> > > > > > > +++ b/gcc/c/c-tree.h
> > > > > > > @@ -147,6 +147,11 @@ struct c_expr
> > > > > > >etc), so we stash a copy here. */
> > > > > > > source_range src_range;
> > > > > > > + /* True iff the sizeof expression was enclosed in parentheses.
> > > > > > > + NB: This member is currently only initialized when
> > > > > > > .original_code
> > > > > > > + is a SIZEOF_EXPR. ??? Add a default constructor to this
> > > > > > > class. */
> > > > > > > + bool parenthesized_p;
> > > > > > > +
> > > > > > > /* Access to the first and last locations within the source
> > > > > > > spelling
> > > > > > >of this expression. */
> > > > > > > location_t get_start () const { return src_range.m_start; }
> > > > > >
> > > > > > I think a magic tree code would be better, c_expr is used in too
> > > > > > many places
> > > > > > and returned by many functions, so it is copied over and over.
> > > > > > Even if you must add it, it would be better to change the struct
> > > > > > layout,
> > > > > > because right now there are fields: tree, location_t, tree,
> > > > > > 2xlocation_t,
> > > > > > which means 32-bit gap on 64-bit hosts before the second tree, so
> > > > > > the new
> > > > > > field would fit in there. But, if it is mostly uninitialized, it
> > > > > > is kind of
> > > > > > unclean.
> > > > >
> > > > > Ok, here's a version with PAREN_SIZEOF_EXPR. It doesn't require
> > > > > changes to
> > > > > c_expr, but adding a new tree code is always a pain...
> > > > >
> > > > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
> > > > >
> > > > > -- >8 --
> > > > > This patch implements a new warning, -Wsizeof-array-div. It warns
> > > > > about
> > > > > code like
> > > > >
> > > > >int arr[10];
> > > > >sizeof (arr) / sizeof (short);
> > > > >
> > > > > where we have a division of two sizeof expressions, where the first
> > > > > argument is an array, and the second sizeof does not equal the size
> > > > > of the array element. See e.g.
> > > > >
Re: [PATCH v3] c, c++: Implement -Wsizeof-array-div [PR91741]
Ping for the C parts.
On Mon, Sep 28, 2020 at 02:15:41PM -0400, Marek Polacek via Gcc-patches wrote:
> On Tue, Sep 22, 2020 at 04:07:41PM -0400, Jason Merrill wrote:
> > On 9/22/20 1:29 PM, Marek Polacek wrote:
> > > Ping.
> >
> > The C++ change is OK.
>
> Ping for the C parts.
>
> > > On Tue, Sep 15, 2020 at 04:33:05PM -0400, Marek Polacek via Gcc-patches
> > > wrote:
> > > > On Tue, Sep 15, 2020 at 09:04:41AM +0200, Jakub Jelinek via Gcc-patches
> > > > wrote:
> > > > > On Mon, Sep 14, 2020 at 09:30:44PM -0400, Marek Polacek via
> > > > > Gcc-patches wrote:
> > > > > > --- a/gcc/c/c-tree.h
> > > > > > +++ b/gcc/c/c-tree.h
> > > > > > @@ -147,6 +147,11 @@ struct c_expr
> > > > > >etc), so we stash a copy here. */
> > > > > > source_range src_range;
> > > > > > + /* True iff the sizeof expression was enclosed in parentheses.
> > > > > > + NB: This member is currently only initialized when
> > > > > > .original_code
> > > > > > + is a SIZEOF_EXPR. ??? Add a default constructor to this
> > > > > > class. */
> > > > > > + bool parenthesized_p;
> > > > > > +
> > > > > > /* Access to the first and last locations within the source
> > > > > > spelling
> > > > > >of this expression. */
> > > > > > location_t get_start () const { return src_range.m_start; }
> > > > >
> > > > > I think a magic tree code would be better, c_expr is used in too many
> > > > > places
> > > > > and returned by many functions, so it is copied over and over.
> > > > > Even if you must add it, it would be better to change the struct
> > > > > layout,
> > > > > because right now there are fields: tree, location_t, tree,
> > > > > 2xlocation_t,
> > > > > which means 32-bit gap on 64-bit hosts before the second tree, so the
> > > > > new
> > > > > field would fit in there. But, if it is mostly uninitialized, it is
> > > > > kind of
> > > > > unclean.
> > > >
> > > > Ok, here's a version with PAREN_SIZEOF_EXPR. It doesn't require
> > > > changes to
> > > > c_expr, but adding a new tree code is always a pain...
> > > >
> > > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
> > > >
> > > > -- >8 --
> > > > This patch implements a new warning, -Wsizeof-array-div. It warns about
> > > > code like
> > > >
> > > >int arr[10];
> > > >sizeof (arr) / sizeof (short);
> > > >
> > > > where we have a division of two sizeof expressions, where the first
> > > > argument is an array, and the second sizeof does not equal the size
> > > > of the array element. See e.g.
> > > >
Re: [PATCH v3] c, c++: Implement -Wsizeof-array-div [PR91741]
On Tue, Sep 22, 2020 at 04:07:41PM -0400, Jason Merrill wrote:
> On 9/22/20 1:29 PM, Marek Polacek wrote:
> > Ping.
>
> The C++ change is OK.
Ping for the C parts.
> > On Tue, Sep 15, 2020 at 04:33:05PM -0400, Marek Polacek via Gcc-patches
> > wrote:
> > > On Tue, Sep 15, 2020 at 09:04:41AM +0200, Jakub Jelinek via Gcc-patches
> > > wrote:
> > > > On Mon, Sep 14, 2020 at 09:30:44PM -0400, Marek Polacek via Gcc-patches
> > > > wrote:
> > > > > --- a/gcc/c/c-tree.h
> > > > > +++ b/gcc/c/c-tree.h
> > > > > @@ -147,6 +147,11 @@ struct c_expr
> > > > >etc), so we stash a copy here. */
> > > > > source_range src_range;
> > > > > + /* True iff the sizeof expression was enclosed in parentheses.
> > > > > + NB: This member is currently only initialized when
> > > > > .original_code
> > > > > + is a SIZEOF_EXPR. ??? Add a default constructor to this class.
> > > > > */
> > > > > + bool parenthesized_p;
> > > > > +
> > > > > /* Access to the first and last locations within the source
> > > > > spelling
> > > > >of this expression. */
> > > > > location_t get_start () const { return src_range.m_start; }
> > > >
> > > > I think a magic tree code would be better, c_expr is used in too many
> > > > places
> > > > and returned by many functions, so it is copied over and over.
> > > > Even if you must add it, it would be better to change the struct layout,
> > > > because right now there are fields: tree, location_t, tree,
> > > > 2xlocation_t,
> > > > which means 32-bit gap on 64-bit hosts before the second tree, so the
> > > > new
> > > > field would fit in there. But, if it is mostly uninitialized, it is
> > > > kind of
> > > > unclean.
> > >
> > > Ok, here's a version with PAREN_SIZEOF_EXPR. It doesn't require changes
> > > to
> > > c_expr, but adding a new tree code is always a pain...
> > >
> > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
> > >
> > > -- >8 --
> > > This patch implements a new warning, -Wsizeof-array-div. It warns about
> > > code like
> > >
> > >int arr[10];
> > >sizeof (arr) / sizeof (short);
> > >
> > > where we have a division of two sizeof expressions, where the first
> > > argument is an array, and the second sizeof does not equal the size
> > > of the array element. See e.g.
> > >
Re: [PATCH v3] c, c++: Implement -Wsizeof-array-div [PR91741]
On 9/22/20 1:29 PM, Marek Polacek wrote:
Ping.
The C++ change is OK.
On Tue, Sep 15, 2020 at 04:33:05PM -0400, Marek Polacek via Gcc-patches wrote:
On Tue, Sep 15, 2020 at 09:04:41AM +0200, Jakub Jelinek via Gcc-patches wrote:
On Mon, Sep 14, 2020 at 09:30:44PM -0400, Marek Polacek via Gcc-patches wrote:
--- a/gcc/c/c-tree.h
+++ b/gcc/c/c-tree.h
@@ -147,6 +147,11 @@ struct c_expr
etc), so we stash a copy here. */
source_range src_range;
+ /* True iff the sizeof expression was enclosed in parentheses.
+ NB: This member is currently only initialized when .original_code
+ is a SIZEOF_EXPR. ??? Add a default constructor to this class. */
+ bool parenthesized_p;
+
/* Access to the first and last locations within the source spelling
of this expression. */
location_t get_start () const { return src_range.m_start; }
I think a magic tree code would be better, c_expr is used in too many places
and returned by many functions, so it is copied over and over.
Even if you must add it, it would be better to change the struct layout,
because right now there are fields: tree, location_t, tree, 2xlocation_t,
which means 32-bit gap on 64-bit hosts before the second tree, so the new
field would fit in there. But, if it is mostly uninitialized, it is kind of
unclean.
Ok, here's a version with PAREN_SIZEOF_EXPR. It doesn't require changes to
c_expr, but adding a new tree code is always a pain...
Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
-- >8 --
This patch implements a new warning, -Wsizeof-array-div. It warns about
code like
int arr[10];
sizeof (arr) / sizeof (short);
where we have a division of two sizeof expressions, where the first
argument is an array, and the second sizeof does not equal the size
of the array element. See e.g.
Re: [PATCH v3] c, c++: Implement -Wsizeof-array-div [PR91741]
Ping.
On Tue, Sep 15, 2020 at 04:33:05PM -0400, Marek Polacek via Gcc-patches wrote:
> On Tue, Sep 15, 2020 at 09:04:41AM +0200, Jakub Jelinek via Gcc-patches wrote:
> > On Mon, Sep 14, 2020 at 09:30:44PM -0400, Marek Polacek via Gcc-patches
> > wrote:
> > > --- a/gcc/c/c-tree.h
> > > +++ b/gcc/c/c-tree.h
> > > @@ -147,6 +147,11 @@ struct c_expr
> > > etc), so we stash a copy here. */
> > >source_range src_range;
> > >
> > > + /* True iff the sizeof expression was enclosed in parentheses.
> > > + NB: This member is currently only initialized when .original_code
> > > + is a SIZEOF_EXPR. ??? Add a default constructor to this class. */
> > > + bool parenthesized_p;
> > > +
> > >/* Access to the first and last locations within the source spelling
> > > of this expression. */
> > >location_t get_start () const { return src_range.m_start; }
> >
> > I think a magic tree code would be better, c_expr is used in too many places
> > and returned by many functions, so it is copied over and over.
> > Even if you must add it, it would be better to change the struct layout,
> > because right now there are fields: tree, location_t, tree, 2xlocation_t,
> > which means 32-bit gap on 64-bit hosts before the second tree, so the new
> > field would fit in there. But, if it is mostly uninitialized, it is kind of
> > unclean.
>
> Ok, here's a version with PAREN_SIZEOF_EXPR. It doesn't require changes to
> c_expr, but adding a new tree code is always a pain...
>
> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
>
> -- >8 --
> This patch implements a new warning, -Wsizeof-array-div. It warns about
> code like
>
> int arr[10];
> sizeof (arr) / sizeof (short);
>
> where we have a division of two sizeof expressions, where the first
> argument is an array, and the second sizeof does not equal the size
> of the array element. See e.g.
Re: [PATCH v3] c, c++: Implement -Wsizeof-array-div [PR91741]
On Tue, Sep 15, 2020 at 09:04:41AM +0200, Jakub Jelinek via Gcc-patches wrote:
> On Mon, Sep 14, 2020 at 09:30:44PM -0400, Marek Polacek via Gcc-patches wrote:
> > --- a/gcc/c/c-tree.h
> > +++ b/gcc/c/c-tree.h
> > @@ -147,6 +147,11 @@ struct c_expr
> > etc), so we stash a copy here. */
> >source_range src_range;
> >
> > + /* True iff the sizeof expression was enclosed in parentheses.
> > + NB: This member is currently only initialized when .original_code
> > + is a SIZEOF_EXPR. ??? Add a default constructor to this class. */
> > + bool parenthesized_p;
> > +
> >/* Access to the first and last locations within the source spelling
> > of this expression. */
> >location_t get_start () const { return src_range.m_start; }
>
> I think a magic tree code would be better, c_expr is used in too many places
> and returned by many functions, so it is copied over and over.
> Even if you must add it, it would be better to change the struct layout,
> because right now there are fields: tree, location_t, tree, 2xlocation_t,
> which means 32-bit gap on 64-bit hosts before the second tree, so the new
> field would fit in there. But, if it is mostly uninitialized, it is kind of
> unclean.
Ok, here's a version with PAREN_SIZEOF_EXPR. It doesn't require changes to
c_expr, but adding a new tree code is always a pain...
Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
-- >8 --
This patch implements a new warning, -Wsizeof-array-div. It warns about
code like
int arr[10];
sizeof (arr) / sizeof (short);
where we have a division of two sizeof expressions, where the first
argument is an array, and the second sizeof does not equal the size
of the array element. See e.g.
