Re: [PATCH v2 08/10] target/hexagon: import parser for idef-parser

[Alessandro Di Federico]

On Thu, 25 Feb 2021 19:30:14 -0800
Richard Henderson <richard.henderson@linaro.org> wrote:

> > +    }
> > +}
> > +code
> > +{
> > +    if (c->inst.error_count != 0) {
> > +        fprintf(stderr,
> > +                "Parsing of instruction %s generated %d errors!\n",
> > +                c->inst.name,
> > +                c->inst.error_count);
> > +        EMIT(c, "assert(false && \"This instruction is not
> > implemented!\");");  
> 
> What's the point of assert(false) above abort()?
> 
> Is there any point in emitting anything at all, since I assume the
> idef-parser program itself will exit with error, stopping the build
> process?

This is a leftover, that string will never be written to disk (`commit`
is not invoked).

> > +| pre ASSIGN rvalue
> > +{
> > +    @1.last_column = @3.last_column;  
> 
> Do you really find any value in this column manipulation, given that
> the input is not the original file, but the output of cpp?

The output of `cpp` is quite readable. We use it a lot for debugging
and it's very helpful.

> IMO this is another reason to *not* preprocess with macros.inc, nor
> sed the output as a workaround for your parsing troubles.

Yes, `sed` is a workaround I really don't like too. But preprocessing
with `cpp` saves us from having to handle a larger, redundant language.
After all, the input language is designed to be expanded through the
preprocessor, although with a different set of macros. I'd keep that
part.

-- 
Alessandro Di Federico
rev.ng