Re: [PATCH] document IntegerRange in internals manual

[Martin Liška <mliska at suse dot cz>]

On 07/10/2017 05:08 PM, Martin Sebor wrote:
> On 07/10/2017 02:35 AM, Martin Liška wrote:
> > On 07/07/2017 09:20 PM, Martin Sebor wrote:
> > > A conflict in my patch for bug 81345 made me notice that r249734
> > > recently added a new option property, IntegerRange.  The change
> > > below adds brief documentation of the property to the manual.
> > >
> > > Martin, can you please check to make sure I didn't miss anything?
> > >
> > > Btw., while experimenting with the property I noticed that there
> > > is no error when option that specifies IntegerRange is set in
> > > the .opt file to a value outside that range.  Would it be hard
> > > to add some checks the the awk scripts to validate that the
> > > argument values are in the range?  It might help avoid bugs
> > > similar to 81345).
> >
> > Sure, please take a look at attached patch. Can you please test it?
>
> The detection works fine for the Init problem (thanks!) but it
> doesn't catch the out-of-range initializer in LangEnabledBy(C,
> Wall, 2, 0) or in Alias(Wfoobar=, 1, 0).  I don't know enough
> about the option scripts yet to gauge how difficult handling
> these might be.  Do you have any idea?  If you think it's
> doable but outside the scope of this tweak let me know and I'll
> open a bug for it to help us remember to handle it at some point
> too.

Well, it's definitely doable, but doing that in the current awk script is quite cumbersome.
I would prefer to have the option generation rewritten e.g. in Python where current awk
script is not nice to reuse an already parsed information. More class-oriented approach would
be desired here.

Please create a PR, I maybe rewrite it in future if we can benefit from that.
Are you interested in the current patch that handles 'Init' directive to go in?

Martin

> > > By the way of an example, the following invalid specification
> > > is accepted but then causes errors when GCC runs.
> > >
> > > Wfoobar
> > > C ObjC C++ ObjC++ Warning Alias(Wfoobar=, 1, 0)
> > >
> > > Wfoobar=
> > > C ObjC C++ ObjC++ Joined RejectNegative UInteger Var(warn_foobar) Warning LangEnabledBy(C ObjC C++ ObjC++, Wall, 2, 0) Init (7) IntegerRange(3, 5)
> >
> > Here one needs to have 'Init (7)' without space!
>
> Ugh.  I only recently realized this but keep forgetting.  It
> seems like another unnecessary trap that would be nice to fix
> at some point.
>
> Thanks
> Martin
>
> > > Martin
> > >
> > > diff --git a/gcc/doc/options.texi b/gcc/doc/options.texi
> > > index 3b68aab..af56e9f 100644
> > > --- a/gcc/doc/options.texi
> > > +++ b/gcc/doc/options.texi
> > > @@ -264,6 +264,12 @@ option handler.  @code{UInteger} should also be used on options like
> > >  @code{-falign-loops}=@var{n} are supported to make sure the saved
> > >  options are given a full integer.
> > >
> > > +@item IntegerRange(@var{min}, @var{max})
> > > +The option's integer argument is expected to be in the range specified
> > > +by @var{min} and @var{max}, inclusive.  The option parser will check
> > > +and reject option arguments that are outside the range before passing
> > > +it to the relevant option handler.
> >
> > LGTM, thanks for the documentation entry.
> >
> > Martin
> >
> > > +
> > >  @item ToLower
> > >  The option's argument should be converted to lowercase as part of
> > >  putting it in canonical form, and before comparing with the strings