This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [patch] support for -fdump-ada-spec
> The main manual (invoke.texi) needs to document the new option with a
> cross-reference to the Ada manual.
Fair enough, I'll add that.
> As I understand it, this feature
>
> (a) is deliberately limited to a subset of C and C++ declarations and
> macros;
Right.
> (b) may, for some input, generate invalid Ada output rather than a
> diagnostic.
Right, the aim is to help developers doing most of the tedious work of
generating bindings rather than doing it manually, the intent is not to
aim at a 100% solution which would require a full C to Ada compiler.
> If this is as intended it doesn't itself block the feature, but it would
> seem like a good idea to have documentation of what is / is not supported.
That part in itself is actually hard to do since by nature, the light approach
taken allows for some "holes", without necessarily knowing them all, although
I'll try to add some mentions of unsupported features, and in particular
explain that the translation of macros is pretty limited currently, more
on that below.
> The driver changes are OK once the rest of the changes are OK. The
> following is not an exhaustive review of the C changes.
OK, thanks for the review, I'll work on a new version taking your comments
into account.
> It looks like you will always output extended identifiers as UTF-8. Is
> this a form that will always be acceptable as input to the Ada compiler?
Yes, GNAT has a -gnatW8 switch to take identifiers as UTF-8.
> I can see no checks that you don't overflow these fixed-width buffers.
> Buffer overruns are never OK; you should preferably calculate the memory
> needed and always allocate enough (that might need two passes, one to
> calculate and one to write the output after allocation), but failing that
> make sure to avoid buffer overruns.
Fair enough.
> > + case '<':
> > + if (prev + 2 == buffer && prev [1] == '<')
> > + {
> > + if (prev [-1] == '1')
> > + prev [-1] = '2';
> > + else if (prev [-1] == ' '
> > + && prev [-2] == '1')
> > + prev [-2] = '2';
> > +
> > + prev [0] = '*';
> > + prev [1] = '*';
> > + }
>
> Quite what is this code meant to be doing?
Transform 1 << N into 2 ** N, I'll add a comment.
> > + case '0':
> > + if (prev + 1 < buffer && prev [1] == 'x')
> > + {
> > + /* convert 0x.... into 16#....# */
>
> Don't you need to handle 0X the same as 0x, and handle initial 0, not 0x
> or 0X, on integer but not floating-point constants, as octal? (Then there
> are GNU C binary constants starting 0b.)
We don't "need" to handle all these forms since we're not aiming at
transforming 100% of all possible cases, this code (in particular the
macro transformation) is only handling the most common cases.
I'll document that for now as part of the limitations, and put as a TODO
to add this support, since it shouldn't be too hard to add.
> > + if (buffer [-1] == 'L'
> > + || buffer [-1] == 'l'
> > + || buffer [-1] == 'U')
>
> In both these cases, you can have suffixes such as "UL" or "LLU".
Right, same answer as above: that can certainly be put on the TODO list
and be left "unsupported" for now (and documented as such, since now
identified).
> In essence, the point with all these cases is that what you are doing with
> taking a C string representation of the macro expansion and trying to
> convert it to an Ada string seems very fragile.
Right, the macro is certainly much more fragile than the 'node' part,
the idea is really to try to handle common simple cases for now such as
#define X 12
#define Y "foo"
and not much more, and possibly enhance this later.
The idea behind this patch is that the capability is already useful as is,
and it's much easier to enhance it by steps once it's there (and I expect
other people from the Ada community, possibly e.g. Laurent Guerby, to
be able to contribute improvements to this capability once committed).
> It would be better to take the actual tokens, interpret their semantics and
> produce equivalent Ada tokens - call cpp_classify_number then the various
> subsequent functions the C lexer uses to generate trees for the constants,
> then write out an Ada constant with the same value.
Possibly, although this sounds like much more work. It certainly sounds
like in the medium term, that would indeed be the way to go though.
Would you consider this point blocking for an initial integration? If so
I guess the other option would be to remove completely support for macros
as a first step and get the support for nodes in, and then work on redoing
the macro support.
In other words, it is kind of "expected" that the macro translation isn't
very powerful right now, and I'll certainly make sure this is properly
documented as part of the limitations.
> > + /* Special case null for now, need to find a more generic approach */
> > + if (!strcasecmp ((const char *)NODE_NAME (node), "null"))
> > + supported = 0;
>
> You have an array ada_reserved later in this code....
Right, this code can actually be removed completely, since we're now using
to_ada_name to handle this as you noted. I'll remove it.
I'll work on the other comments, thanks again for your review.
Arno