Re: [PATCH 9/9] C++ compile support

Tom Tromey
2018-08-12T00:17:02Z

>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:

Keith> This patch adds *basic* support for C++ to the compile feature. It does
Keith> most simple type conversions, including everything that C compile does and
Keith> your basic "with-classes" type of C++.

Thanks for doing this. This is very good.

Keith> +extern compile_instance *cplus_get_compile_context (void);

I think we stopped using (void) now.

Keith> +typedef compile_program<compile_cplus_instance,
Keith> + cplus_push_user_expression, cplus_pop_user_expression,
Keith> + cplus_add_code_header, c_add_code_footer,
Keith> + cplus_add_input> cplus_compile_program;
Keith> +

Haha, this seemed like overkill to me, but I see it's already this way,
and it doesn't really matter I think.

Keith> diff --git a/gdb/compile/compile-cplus-support.c b/gdb/compile/compile-cplus-support.c
Keith> new file mode 100644
Keith> index 0000000000..42fc7692c4
[...]
Keith> +void
Keith> +gcc_cplus_enter_scope (void *datum, struct gcc_cp_context *gcc_context)
Keith> +{
Keith> +}
Keith> +
Keith> +void
Keith> +gcc_cplus_leave_scope (void *datum, struct gcc_cp_context *gcc_context)
Keith> +{
Keith> +}

Is this something that gets expanded in later patches?

If not, I'd suggest just making these private static methods on
compile_cplus_instance, to avoid needing a new file.

On the other hand, if this is expanded in the future, then it is totally
fine like this.

Keith> +/* Convert a given symbol, SYM, to the compiler's representation.
Keith> + INSTANCE is the compiler instance. IS_GLOBAL is true if the
Keith> + symbol came from the global scope. IS_LOCAL is true if the symbol
Keith> + came from a local scope. (Note that the two are not strictly
Keith> + inverses because the symbol might have come from the static
Keith> + scope.) */
Keith> +
Keith> +static void
Keith> +convert_one_symbol (compile_cplus_instance *instance,
Keith> + struct block_symbol sym, bool is_global, bool is_local)

I think back in the day, I'd hoped that there would be more code sharing
between the C and C++ compile paths. But maybe this wasn't possible? I
didn't do a deep comparison or anything. And it's fine if they have to
be separate.

Keith> +void _initialize_compile_cplus_symbols (void);

I think this declaration isn't needed any more.

Keith> +/* See description in compile-cplus.h. */
Keith> +
Keith> +gdb::unique_xmalloc_ptr<char>
Keith> +decl_name (const char *natural)

This should return a std::string. See below.

Keith> + std::string symbol = (comp.bsymbol.symbol != nullptr
Keith> + ? SYMBOL_NATURAL_NAME (comp.bsymbol.symbol)
Keith> + : "<none>");

This could just be a const char * to avoid an allocation.

Keith> +/* Utility function to convert CODE into a string. */
Keith> +
Keith> +static const char *
Keith> +type_code_to_string (enum type_code code)
Keith> +{
Keith> + const char * const s[] =
Keith> + {"BISTRING (deprecated)", "UNDEF (not used)",
Keith> + "PTR", "ARRAY", "STRUCT", "UNION", "ENUM",
Keith> + "FLAGS", "FUNC", "INT", "FLT", "VOID",
Keith> + "SET", "RANGE", "STRING", "ERROR", "METHOD",
Keith> + "METHODPTR", "MEMBERPTR", "REF", "RVALUE_REF", "CHAR", "BOOL",
Keith> + "COMPLEX", "TYPEDEF", "NAMESPACE", "DECFLOAT", "MODULE",
Keith> + "INTERNAL_FUNCTION", "XMETHOD"};
Keith> +
Keith> + return s[code + 1];

This could get out of sync with gdbtypes.h. So I'd suggest either (1)
taking the simple route and printing this as an integer; or (2) moving
the enum to a .defs file and auto-generating the strings from it, to
avoid any possibility of problems. In case 2, type_code_to_string could
move to gdbtypes.c.

Keith> + scope_component comp
Keith> + = {
Keith> + decl_name (TYPE_NAME (type)).release (),
Keith> + lookup_symbol (TYPE_NAME (type), block (), VAR_DOMAIN, nullptr)

scope_component::name is a std::string, so I think this decl_name call
will leak memory; which is why decl_name should return a std::string
itself.

Keith> +/* Return the declaration name of the symbol named NATURAL.
Keith> + This returns a name with no function arguments or template parameters,
Keith> + suitable for passing to the compiler plug-in. */
Keith> +
Keith> +gdb::unique_xmalloc_ptr<char> decl_name (const char *natural);

This seems like a fairly generic name for something that is maybe
specific to the C++ compile code.

Tom