This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [patch] static_kind -> bit0, bit1 [Re: [gdb] Fortran dynamic arrays]
> 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