On 4 August 2014 16:52, Ian Lance Taylor <i...@google.com> wrote:
> On Sun, Aug 3, 2014 at 11:12 AM, Iain Buclaw <ibuc...@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
