Re: [PATCH, RFC]: Next stage1, refactoring: propagating rtx subclasses

[Mikhail Maltsev <maltsevm at gmail dot com>]

On 28.04.2015 16:46, Richard Sandiford wrote:
>> +inline void rtx_jump_insn::set_jump_target (rtx_insn *target) +{
>> + JUMP_LABEL(this) = target;
Fixed.

> Could these two operate on rtx_code_labels rather than rtx_insns?
Indeed, right now [set_]jump_target are not used with NOTE's, so I could
promote the type to rtx_code_label with no regressions.

> Long line -- can break before "(CASE_LABEL"
Fixed. Others too. Rechecked with contrib/check_GNU_style.sh (no
warnings, except one special case in gcc/recog.h).

On 28.04.2015 20:08, Jeff Law wrote:
> Agreed.  These corner cases are precisely the kinds of things we
> want to be identifying and cleaning up as we go.   I haven't looked
> at the updated patch yet, but if there isn't a comment about this
> case, then there should be.
Done.

> Needs a ChangeLog.  I know it's a bit tedious...  But please include
> it.  It makes patch review easier, and we need one for the ChangeLog
> file anyway.
Attached.

> In general, probably more as_a conversions than I'd like.  But that
> may be unavoidable at this point.  On a positive note, I do see some
> going away as you strengthen various return and parameter types.

Also some conversions are, perhaps, unavoidable when they come right
after a check, so the type is indeed narrowed.

> I probably would have done separate patches for the std::swap
> changes. They're not really related to the rtx subclasses work.
OK, sending 2 separate patches. Note that they a not "commutative":
std::swap should be applied before the main one, because one of the
swaps in do_compare_rtx_and_jump uses a single temporary variable of
type rtx for swapping labels and for storing generic rtl expressions
(this could be worked around, of course, but I think that would be just
a waste of time).

> I'm not sure why you added indention in expand_expr_real_2's switch
> statement.  It certainly makes the patch harder to review.  I'm going
> to assume there was a good reason for the new {} pair and added
> indention.
Sorry for that. I had to introduce a couple of variables, and I decided
to limit their scope in order to make their use less error-prone.


-- 
Regards,
    Mikhail Maltsev

Attachment: as_insn3.cl
Description: Text document

Attachment: as_insn3.patch
Description: Text document

Attachment: swaps.patch
Description: Text document

Attachment: swaps.cl
Description: Text document