Re: [PATCH] New C++ abstraction patch

[Daniel Berlin <dberlin at redhat dot com>]

Michael Elizabeth Chastain <chastain@cygnus.com> writes:

> Hi Daniel,
> 
> I have some comments on your patch.
> 
> Michael
> 
> ===
> 
> + gnu-v3-abi.o: gnu-v3-abi.c cp-abi.h
> + 
> + gnu-v2-abi.o: gnu-v2-abi.c cp-abi.h
> + 
> 
> gnu-v3-abi.o and gnu-v2-abi.o also depend on
> $(defs_h) gdb_regex.h gdb_string.h.
> 

Okey-dokey.

> 
> 
> + int gnuv2_vtable_prefix_p (const char * name)
> + {
> +   return (((name)[0] == '_' 
> + 	   && (((name)[1] == 'V' && (name)[2] == 'T') 
> + 	       ||  ((name)[1] == 'v' && (name)[2] == 't'))
> + 	   && is_cplus_marker ((name)[3])) || 
> + 	  ((name)[0] == '_' && (name)[1] == '_' 
> + 	   && (name)[2] == 'v' && (name)[3] == 't' && (name)[4] == '_'));
> + }
> 
> Would you consider writing this as:
> 
>   int gnuv2_vtable_prefix_p (const char * name)
>   {
>     if (strncmp (name, "_VT", 3) == 0 && is_cplus_marker (name[3]))
>       return 1;
>     if (strncmp (name, "_vt", 3) == 0 && is_cplus_marker (name[3]))
>       return 1;
>     if (strncmp (name, "__vt_", 5) == 0)
>       return 1;
>     return 0;
>   }
> 
> Or if that is not to your taste, something that is more readable than
> the former macro?

Sure, that's fine, I was just cutting and pasting the v2 stuff, since i
was more concerned with the v3 implementation stuff.


> 
> (This is a style issue rather than a correctness issue so I would
> accept whatever you decide to write, including the original).
> 

Hey, i'm happy to do whatever makes it easier 
> 
> 
> + //struct cp_abi_ops current_cp_abi;
> 
> This would be the first use of // comments in gdb.  There are some
> instances of '//', but they are already inside comments, or they are
> part of /* ... *//* ... */ constructions.

Whoops, I forgot to take that out (As I said, i was switching back and
forth by hand).

> 
> 
> 
> + /* FIXME: Stop doing the re_comp's everytime. Use the other regex api
> +    and compile the patterns ahead of time */
> 
> One of the other uses of re_comp/re_exec is in solib_add, which calls
> symbol_file_add in a loop.  I think you're in danger of overwriting the
> single global re_comp buffer.

I was unware.
I'll rewrite it.
> 
> There are other uses in target-specific solib loaders, like irix and hpux.
> 
> I like regular expressions but I think you need regcomp/regexec/regerror
> rather than re_comp/re_exec.

I do too, I was just trying to avoid having to relearn the horrible.

> 
> 
> 
> + int gnuv3_destructor_prefix_p (const char * name)
> + {
> +   re_comp ("[^0-9]D[1-3]Ev");
> +   return re_exec (name) != 0;
> + }
> 
> libiberty/cp-demangle.c says that dtor-names are D0 D1 D2, not D1 D2
> D3.
You are correct.

I wonder what possessed them to use C1,C2,C3, then D0,D1,D2.

> 
> What happens if the target program has a method named TOORAD2Evade?


> I think your r.e. would match that mangled name.
> 
Yes. There is nothing I can do about this.
There is no unambiguous destructor prefix.

Welcome to "Hey, let's hardcode this stuff in" hell.

I plan on using [^0-9]D[1-3]Ev$

So if you had TOORAD2Ev, you'd match.

I could also demangle the name, and check for the "~", but without my
ternary search tree stuff that caches demangled name, this would be a
*serious* performance hit.

I could also use [^0-9]D[1-3]Ev$, and if it matches, demangle, then
check for certain.

This would be much better, performance wise.  

> 
> 
> + int gnuv3_vtable_prefix_p (const char * name)
> + {
> +   return strncmp (name, "_ZTV", 4) != 0;
> + }
> 
> This strncmp() != 0 should be strcmp() == 0.  (silly strncmp)

Right again.
Let me rewrite the regex stuff.

--Dan