> +  for (elem = other_queue; elem ; elem = elem->next)
> +  {

Note that your indentation is off in places.

> +      /* We are parsing DEFINE_SUBST_ATTR, which could cause generation
> +      of DEFINE_ATTR for introduced DEFINE_SUBST.  It doesn't happen
> +      if such DEFINE_ATTR has already been introduced.
> +      If this did happen, we need to return TRUE to process newly
> +      introduced DEFINE_ATTR.  */

This comment is less than clear.  Unclear enough that I don't even
know how to suggest re-wording it.  We're returning true so that the
new define_subst_attr is processed, or that some define_subst is processed?

> +       while ((start = strchr (end, '<')) && (end  = strchr (end, '>')))
> +         {
> +           if (start && end
> +               && (end - start - 1 > 0)
> +               && (end - start - 1 < (int)sizeof (tmpstr)))

That doesn't look right.
(1) end search should be from start, not from the previous end.
(2) you know start and end are non-null inside the loop; why are
    you checking for it again.

The bulk of the patch is looking pretty decent now.  We may find problems
with it when we start to use it, but at the moment it's fairly hard to
evaluate completely.

---

Looking at all this, I'm wondering if we shouldn't split out all of this
macro/include processing to a separate pass.  Perform the preprocessing
once, early, leaving the processed result in the build directory.  Then
run the original/traditional rtl reader on that when running the other
rtl manipulation passes.

The reason being that it's going to become increasingly hard to figure
out if the reason for an error is in the macro processing or in the source
md file.  Being able to see the macro expansion is going to be important.

Of course, another way to get this macro expansion is to leave the macro
processing where it is and have another gen program that merely dumps the
processed rtl.  It wouldn't be run normally, but a makefile target used
for debugging would be sufficient.

---

Which brings us to the question of what to do with the patch for 4.8.
It's true that you made the deadline for stage1 closure.  But there will
be no users of this feature, so it begs the question of why we should
apply it now.  Have you a convincing reason?

RM's do you have an opinion here?


r~

Reply via email to