Again, sorry for the delay, and thank you for the continued pings. Did you see my earlier review of the dwarf2out changes?

On 09/23/2016 08:41 PM, Alexandre Oliva wrote:
+/* Scopes (functions, classes, or templates) in the TREE_VALUE of
+   GLOBAL_FRIEND_LIST are regarded as friends of every class.  This is
+   mainly used by libcc1, to enable GDB's code snippets to access
+   private members without disabling access control in general, which
+   could cause different template overload resolution results when
+   accessibility matters (e.g. tests for an accessible member).  */
+
+static tree global_friend_list;

This should be a hash_set rather than a TREE_LIST.

+  // FIXME: we need a more space-efficient representation for
+  // oracle_looked_up.

A hash_set would work for that, too.

@@ -4626,6 +4647,8 @@ qualified_lookup_using_namespace (tree name, tree scope,
   /* Look through namespace aliases.  */
   scope = ORIGINAL_NAMESPACE (scope);

+  query_oracle (name);

How does this work without providing the scope? Does the oracle provide all definitions of the name?

+/* Exported wrapper for cp_literal_operator_id.  */
+
+tree
+ansi_litopname (const char *name)
+{
+  return cp_literal_operator_id (name);
+}

Why not export cp_literal_operator_id itself?

+   If the newly-created namespace is to be an inline namespace, after
+   push_namespace, get the nested namespace decl with
+   get_current_binding_level, pop back to the enclosing namespace,
+   call using_namespace with INLINE_P, and then push to the inline
+   namespace again.  */

This seems like unnecessary contortions to expect of the caller (i.e. GDB). Let's add a new front end function to handle this.

+     As a general rule, before we can declare or define any local name
+     with a discriminator, we have to at least declare any other
+     occurrences of the same name in the same enclosing entity with
+     lower or absent discriminator.

This seems unfortunate, wouldn't it be better to allow the plugin to specify the discriminator directly?

+.  If the code snippet is at point 2, we don't need to (re)activate
+   anything declaration: nothing from any local scope is visible.
+   Just entering the scope of the class containing member function f
+   reactivates the names of its members, including the class name
+   itself.  */

Does it reactivate the names of other declarations in scope in the enclosing function, e.g. static local variables?

+/* Pop the namespace last entered with push_namespace, or class last
+   entered with push_class, or function last entered with
+   push_function, restoring the binding level in effect before the
+   matching push_* call.  */
+
+GCC_METHOD0 (int /* bool */, pop_namespace)

This should have a different name, perhaps pop_last_entered_scope?

+/* Return the NAMESPACE_DECL, TYPE_DECL or FUNCTION_DECL of the
+   binding level that would be popped by pop_namespace.  */
+
+GCC_METHOD0 (gcc_decl, get_current_binding_level)

Perhaps get_current_binding_level_decl?

+/* Add USED_NS to the namespaces used by the current binding level.
+   Use get_current_binding_level to obtain USED_NS's gcc_decl.
+   INLINE_P indicates USED_NS was declared as an inline namespace, or
+   the presence of attribute strong in the using directive, which
+   is an older but equivalent GCC extension.  */
+
+GCC_METHOD2 (int /* bool */, using_namespace,
+            gcc_decl,                  /* Argument USED_NS.  */
+            int /* bool */)            /* Argument INLINE_P.  */

As above, I'd prefer to hide this implementation detail from GDB in favor of indicating when we push into an inline namespace.

+   The TARGET decl names the qualifying scope (foo:: above) and the
+   identifier (bar), but that does not mean that only TARGET will be
+   brought into the current scope: all bindings of TARGET's identifier
+   in the qualifying scope will be brought in.

This seems wrong; for namespace-scope using-declarations, only the declarations in scope at the point of the using-declaration are imported. Since DWARF represents each imported declaration individually, I would think that we would want the plugin to import them one at a time.

+   Note that, since access controls are disabled, we have no means to
+   express private, protected and public.

Is this still true?

For operator functions, set GCC_CP_FLAG_SPECIAL_FUNCTION in
+   SYM_KIND, in addition to any other applicable flags, and pass as
+   NAME a string starting with the two-character mangling for operator
+   name

Perhaps the compiler side should handle this transparently?

+/* Supply the ADDRESS of one of the multiple clones of constructor or
+   destructor CDTOR.  The clone is selected using the following
+   name mangling conventions:

The comment doesn't say what argument NAME should be.

+   The [CD]4 manglings (and symbol definitions) are non-standard, but
+   GCC uses them in some cases: rather than assuming they are
+   in-charge or not-in-charge, they test the implicit argument that
+   the others ignore to tell how to behave.  These are defined in very
+   rare cases of virtual inheritance and cdtor prototypes.  */

These are used instead of cloning when we can't just use aliases.

+   In order to simplify such friend declarations, and to enable
+   incremental friend declarations as template specializations are
+   introduced, new_friend can be called after the befriended class is
+   fully defined, passing it a non-NULL TYPE argument naming the
+   befriended class type.  */

"befriending", not "befriended".

+GCC_METHOD5 (gcc_type, new_template_typename_parm,

s/typename/type/

+GCC_METHOD2 (gcc_type, new_dependent_typespec,

"typespec" usually means type-specifier. How about new_dependent_type_template_id?

+GCC_METHOD5 (gcc_expr, new_dependent_value_expr,

I don't think you need "value" here.  Nor in the other *_value_expr names.

+GCC_METHOD3 (gcc_expr, type_value_expr,

Maybe "cast_expr"?

+/* Build a gcc_expr that denotes the conversion of an expression list
+   VALUES to TYPE, with ("tl") or without ("cv") braces, or a braced
+   initializer list of unspecified type (e.g., a component of another
+   braced initializer list; pass "il" for CONV_OP, and NULL for
+   TYPE).  */

+GCC_METHOD3 (gcc_expr, values_expr,

Hmm, it seems like you're conflating two things here: the expressions, and the conversions. I'd suggest functional_cast_expr, but handling a plain braced-init-list through the same interface is surprising.

+GCC_METHOD4 (gcc_expr, alloc_expr,

new_expr?

This leads me to notice that some of the entry points start with "new_", some start with "build_", and some have no prefix. Is this intentional?

+/* Create a new closure class type, record it as the
+   DISCRIMINATOR-numbered closure type in the current scope (or
+   associated with EXTRA_SCOPE, if non-NULL), and enter the closure
+   type's own binding level.  This primitive would sort of combine
+   new_decl and start_class_definition, if they could be used to
+   introduce a closure type.  Initially it has no fields.

What's the new_decl part?

+   NAME is the class name.  FILENAME and LINE_NUMBER specify the
+   source location associated with the class.  EXTRA_SCOPE, if
+   non-NULL, must be a PARM_DECL of the current function, or a
+   FIELD_DECL of the current class.  If it is NULL, the current scope
+   needs not be a function.  */

I'm not sure what you mean by "needs not" here. I read that as not requiring that the current scope is a function, which seems unnecessary to say.

+/* Create a modified version of a function type that has default
+   values for some of its arguments.  The returned type should ONLY be
+   used to define functions or methods, never to declare parameters,
+   variables, types or the like.

Handling these in the function type seems like an artifact of the GCC implementation of default arguments that needn't be exposed to GDB, particularly since you already also offer set_deferred_function_default_args to apply them to the decl directly.

+/* FIXME: do we need to support argument packs?  */

You shouldn't, they're an internal detail.

+  /* We intentionally cannot express inline, constexpr, friend or
+     virtual override for functions.  We can't inline or
+     constexpr-replace without a source-level body.  Since we disable
+     access control, friend is meaningless.

Is this still true?

Jason

Reply via email to