Re: [patch] static_kind -> bit0, bit1 [Re: [gdb] Fortran dynamic arrays]

[Joel Brobecker <brobecker@adacore.com>]

> I agree "static_kind" is "discriminant" but not "artificial".  Artificial is
> just additional flag existing only for FIELDS of type TYPE_CODE_FUNC or type
> TYPE_CODE_METHOD:

Yes, I don't know why I was thinking that artificial was a discriminant
(meaning "no location").

> Moreover the split into the big union gets more complicated when we merge
> them.  Should I really rework the patch into the enum merging
> artificial+static_kind?

No that won't be necessary. I agree with Daniel that we should go with
your patch for now.  Improving the clarity of the discriminant can be
done separately.

> Sorry, a typo from the ChangeLog script
>   http://sourceware.org/ml/gdb-patches/2006-08/msg00195.html
> , put there at least a warning now:
>   warn "Extra directory" if $dfile=~m{(?:gdb|bfd|opcodes|libiberty)/};

No problem, please just remember to fix these before you commit.

> > > +      lower_bound = f77_get_lowerbound (type);
> > > +      if (lower_bound != 1)	/* Not the default.  */
> > > +	fprintf_filtered (stream, "%d:", lower_bound);
> > 
> > Here, it looks like  we're slightly modifying the behavior - if the lower
> > bound was undefined, then we're throwing an error when we used not to.
> 
> That was a dead code.  BOUND_FETCH_ERROR could never have been returned by
> f77_get_dynamic_lowerbound():
> BOUND_SIMPLE returns BOUND_FETCH_OK.
> BOUND_CANNOT_BE_DETERMINED already error()s itself and never returns.
> Other BOUND_* cases were never set.

Hmmm, you are right! So that piece was fine too.

> Another question is if BOUND_CANNOT_BE_DETERMINED should error() as
> currently does or if it rather should return BOUND_FETCH_ERROR.  But
> that decision is outside of the scope of my patch and changes the
> current GDB behavior.

Absolutely.

> > > +  if (TYPE_CODE (type) == TYPE_CODE_ARRAY)
> > > +    {
> > > +      printfi_filtered (spaces, "upper bound undefined is %d\n",
> > > +			TYPE_ARRAY_UPPER_BOUND_IS_UNDEFINED (type));
> > > +      printfi_filtered (spaces, "lower bound undefined is %d\n",
> > > +			TYPE_ARRAY_LOWER_BOUND_IS_UNDEFINED (type));
> > > +    }
> > 
> > I think this is only repetitive, and shouldn't be displayed. The
> > artificial field value will be displayed a little later, and that
> > should be good enough.  No strong objection, though.
> 
> As I hope the "artificial" field may start to exist only for some types
> I tried to make the patch more abstract to the specific implementation of
> TYPE_ARRAY_{UPPER,LOWER}_BOUND_IS_UNDEFINED.  But no strong opinion on
> either way.

That's only my opinion too, and there is no real reason why my opinion
should count too. But to me, this function does a low-level dump of
our type structure, and thus should follow closely the implementation.
If we start to introduce a higher-level view of this type, I think
it can potentially be confusing.  For now, I would prefer to leave
that part out.

Your patch is approved with the few corrections I mentioned.
Thank you.

-- 
Joel