This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Add D demangling support to libiberty


On 4 August 2014 16:52, Ian Lance Taylor <iant@google.com> wrote:
> On Sun, Aug 3, 2014 at 11:12 AM, Iain Buclaw <ibuclaw@gdcproject.org> wrote:
>>
>> This adds a demangler for the D programming language to libiberty,
>> intended to be used in GDB and Binutils.  GDB already has a trimmed
>> down implementation of this, but have been advised that here would be
>> a better location to house it.
>>
>> Notes that I think are of interest / questions I have about how I've done this.
>>
>> - The implementation is some 1200 SLOC (and may grow), so I've put it
>> in a new file, as opposed included cplus-dem.c.  Is this reasonable?
>
> Yes.
>
>
>> - This borrows and extends the mini string package in cplus-dem.c,
>> because it was the simplest to use when writing this.  The GDB
>> implementation uses obstack, and I'm aware of dyn_string, but I can't
>> say I'm a fan of using either.
>
> One thing we discovered for the C++ demangler is that sometimes it's
> nice to be able to invoke it from locations where the memory allocator
> is not available, such as when the program is crashing.  That is why
> the C++ demangler has a cplus_demangle_v3_callback entry point.  For
> ordinary demangling the code uses growable_string and
> d_growable_string_callback_adapter.  It's not important for now but
> you may want to consider doing something like that in the future.
>
> If you do choose to follow that path, because of namespace
> considerations I think the right approach would be to move the
> growable_string code and the adapter into a .h file in libiberty that
> defines the functions as static.  Then both demanglers could #include
> that file.  That would let them share the code without adding
> unacceptable new symbols to libstdc++.
>
>
>> - GDB has a testsuite that provides most of the coverage for what this
>> code should be doing.
>
> That is nice but we need a small testsuite in libiberty too--see
> libiberty/testsuite/test-demangle.c and demangle-expected.  Ideally
> you should be able to just add some entries to demangle-expected.
>
>

OK, I'll examine this.



>> - List of functions in d-demangle.c can be added to the ChangeLog upon request.
>
> Not necessary.
>
>
>> - I haven't signed any copyright assignments to GCC.  But I have
>> papers from Donald ready to send across.
>
> Definitely necessary before we can consider this.  Please get this
> squared away before proceeding with this patch.  Let us know if you
> need any help with this.
>
>

I'll let you know when it's been processed.  Last time they had a fast
turnaround.


>> +#ifdef HAVE_STDLIB_H
>> +#include <stdlib.h>
>> +#else
>> +long strtol (const char *nptr, char **endptr, int base);
>> +long double strtold (const char *nptr, char **endptr);
>
> Use an explicit "extern".
>
>> +#define ASCII2HEX(c) \
>> +  (('a' <= (c) && (c) <= 'f') ? \
>> +   ((c) - 'a' + 10) \
>> +   : (('A' <= (c) && (c) <= 'F') ? \
>> +      ((c) - 'A' + 10) \
>> +      : (('0' <= (c) && (c) <= '9') ? \
>> +      ((c) - '0') \
>> +      : 0 \
>> +        ) \
>> +     ) \
>> +  )
>
> Use a function, not a macro that multiple-evaluates its argument.
>
>
>> +/* Prototypes for D demangling functions */
>
> Current style is to only forward declare functions when necessary.
>
>
>> +const char *
>> +dlang_call_convention (string *decl, const char *mangled)
>
> This should be explicitly static.  Same applies to many of the
> functions below.
>

I've dealt with the above, but I'll re-submit the patch when I've
added in the testsuite items.

Regards
Iain


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]