Fix use of --schema with --outfile

[jobol]

That change has two parts:

• use the option --outfile when generating binary schema
• avoid the removal of that created file

[mikkelfj]

Thanks, I'll have a look and get back to you.

[jobol]

just a little ping ^_^

[mikkelfj]

Thanks, haven't forgotten, just a little busy.

[mikkelfj]

OK, I looked more closely.

I think the code is probably OK, it is bit complicated admittedly. However, I'm note sure that the --outfile option is really meaningful for binary schema, unless you can also stream bfbs files to stdout.

From the help message: " --outfile=<file> Like --stdout, but to a file.

What happens if both generate ordinary code and the binary schema at the same time and choose --outfile or --stdout? (I don't have all the answers since this was written a long time ago).

[jobol]

ATM in holidays for a week but just few words about your question. If I recall correctly, the case is checked and produce an error that forbids any processing. To be checked. Back near the 8th of november. cheers

[jobol]

Back but seek, I'll check that next week. I admit I have not fully understood your mind until I re-read it, so forget my previous answer.

[jobol]

Finally got time to check.

You are right, --stdout can work with binary schema and produce the expected result.

Though nothing forbids to instead use --outfile that without that patch is buggy.

I checked generation of both schema and regular files. As expected It throws an error diagnostic message. Note that I added \n to the previously existing messages that were missing it. Here the ouput:

$ flatcc --schema -a --outfile=zzz sample.fbs
--outfile cannot be used with mixed text and binary output
output failed
$ flatcc --schema -a --stdout  sample.fbs > zzz
--stdout cannot be used with mixed text and binary output
output failed
$ 

[mikkelfj]

Aside from my comment, this looks fine.

[mikkelfj]

I see. I was thinking of setting a flag in the context to guide generate files, or not call generate files. But there is also the consideration that the call is not from CLI but from the flatcc library.

[mikkelfj]

I need to think a bit about the location of source code, but otherwise I have no objections.

[jobol]

I understand your point. IMHO, It is a tradeoff between clarity and time to spend. Generally, for developers, 2 qualities are fighting: being courageous (when refactoring is needed -as here perhaps-), being lazy (because it can avoid you many uninteresting lines of code and wasting time). Here we have to be courageous but a big comment explaining the issue and telling that it has to be solved later is also possible.

I have not tried to improve the code for that point. As you wrote above, it is a bit complicated. So it asked to be very courageous in the middle of someone else code....

[jobol]

Depletion of human time! I understand that at the end some redesign of option processing would be cool before accepting that change. But who has got the time?

[mikkelfj]

There was once this shop with a sign reading: we do wonders while you wait, miracles take a little longer.

[mikkelfj]

@jobol are you still there?

I'm finally ready to have a look a this. You have a lot of checks on what code is generated in flatcc.c to avoid calling fb_init_output_c function.

My question is: In this function in src/compiler/codegen_c.c there is a check

    if (!out->opts->gen_outfile) {
        /* Normal operation to multiple header filers. */
        return 0;
    }

But the calling function in flatcc.c does care about that. So I wonder if it would make sense to replace your many checks with the bove check, just before calling fb_init_output_c and then return?

To be honest the working logic is not entirely clear to me anymore.

[jobol]

Hello @mikkelfj I'm still here and still interested to see the change pulled. However that is a big jump in the past so, behind the blur about what I exactly did, I only see that it tooks me lot of trials to reach the magic formulae.

So I can't reply anything to your question, perhaps yes, perhaps no...

I'm focused on some other work at the moment but I could find some time next week to check that.

[mikkelfj]

OK, I'll try to make my suggested change in a branch and link here for you to test then. I'm not fully into what you try to fix, so I think it is best you give it a go. The original checks are a bit fragile, and always have been, which is why I don't like them to proliferate.

[mikkelfj]

I added the temp branch https://github.com/dvidelabs/flatcc/tree/bin-outfile It is your PR branch merged with latest master, then a commit added to replace the expression in src/compiler/flatcc.c

The complex boolean expression is now available as P->opts.cgen indicating that source code generation is enabled. cgen is mutually exclusive with binary schema generation and cannot both stream to stdout or a given output file - that is already checked earlier on. Therefore it ought to be sufficient to check for P->opts.bgen_bfbs_gen and the opts.cgen is not necessary to check.

#ifdef PR216
    /* Note: P->opts.cgen now represents the below expression, and bfbs_gen is mutually exclusive with cgen */
    if (!(P->opts.cgen_reader | P->opts.cgen_builder | P->opts.cgen_verifier
        | P->opts.cgen_common_reader | P->opts.cgen_common_builder
        | P->opts.cgen_json_parser | P->opts.cgen_json_printer)) {
        return 0;
    }
#else
    /* Experimental alternative to PR216 */
    if (P->opts.bgen_bfbs) {
        return 0;
    }
#endif

I'm still not sure if this is exactly what we want, but it does clean up the logic a bit.

[mikkelfj]

Merging bin-outfile feature branch to master.

[jobol]

\o/