On Jan 13, 2017, Jason Merrill <ja...@redhat.com> wrote:
> On 09/23/2016 08:41 PM, Alexandre Oliva wrote:
>> +static tree global_friend_list;
> This should be a hash_set rather than a TREE_LIST.
You sure? At least in the libcc1 use scenario, this is going to have a
single entry. I didn't even have to make it a list, but I made it one
because, well, I could :-) A hash_set seems excessive, unless you
envision other uses for it.
>> + // FIXME: we need a more space-efficient representation for
>> + // oracle_looked_up.
> A hash_set would work for that, too.
I was hoping for an unused bit in IDENTIFIERs, but I guess that will do.
I'm a bit concerned about the performance impact of this in all name
lookups, though. Just computing the hash for every lookup seems
excessive, compared with testing a bit. Unless... I guess making it a
hash_set*, NULL unless there is an Oracle, would alleviate this concern,
making it cost only when it's in use.
>> @@ -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?
Yeah, all namespace-scoped definitions. I figured we'd need them all
for the existing lookup code to find the expected results in the
presence of using declarations and whatnot.
>> +/* 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?
My sense of aesthetics demanded a uniform set of exported names along
with ansi_opname and ansi_assopname ;-)
>> + 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.
Heh, it's really nothing compared with some other contortions required
to navigate binding levels, e.g. to reenter a function scope.
I didn't want to add redundant functionality to the libcc1 API, and we'd
need the ability to separate the introduction or reentry in a namespace,
from a using directive with attribute strong. And since inline
namespaces use the same implementation as that, I figured it made sense
to use the same interface for both.
Besides, push_namespace is called a lot more often than using_namespace,
so it makes sense to keep it simpler, and leave the extra parameter for
the less common operation.
Another argument to keep things as they are is that the inline-ness of a
namespace is not a property related with entering the namespace, but
rather with how names from it are brought into the other namespace. So,
in my mind, that's where the flag should go. After all, what would it
mean to enter a namespace without the inline flag that had previously
been entered with the inline flag set? The language specifies that
inline is implicit when left out, but if the libcc1 caller has to
indicate upon every entry whether or not a namespace is to be/remain
inline, that's not only a burden on the caller, but also a conflict
scenario that may have to be detected and have an error flagged within
libcc1. API-wise, that makes a lot less sense to me.
>> + 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?
That would have required a major reworking of the internal handling of
such discriminators, and it didn't seem worth the effort and risk of
pushback, considering that libcc1 already requires all occurrences of a
name to be defined at once.
>> +. If the code snippet is at point 2, we don't need to (re)activate
>> + anything declaration: nothing from any local scope is visible.
s/anything/any/ fixed.
>> + 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?
My understanding is that anything local needs explicit reactivation when
reentering a function scope. That makes sense and is helpful for us,
given that a local scope is not always a single scope; unlike namespace
and class scopes, there could be multiple scopes within a function
scope, and each would have a different set of active names. The client
has to select the active names upon function reentry.
>> +/* 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?
It was introduced very early on, long before the need for exposing
function scopes was realized and implemented. GDB (branch) already had
plenty of code using this primitive, so I didn't want to rename it, but
if you insist in such spelling changes, I won't mind, and I guess GDB
folks won't even. It's not too late yet ;-)
>> +/* 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?
Ditto, except I'm not sure GDB already uses this one, so it's probably a
much easier sell.
>> +/* 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.
You mean hide using directives with attribute strong?
>> + 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.
What you say is true, but GCC doesn't offer functionality for that
AFAICT, and the caller can always arrange for only the bindings that
should be brought in by the using declarations to be defined at the time
the using declaration is introduced, so I left it at that so as to
minimize the changes to GCC proper.
>> + Note that, since access controls are disabled, we have no means to
>> + express private, protected and public.
> Is this still true?
No, thanks for spotting it, fixed.
>> 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?
Sorry, I don't understand what you're suggesting. Can you please
elaborate?
>> +/* 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.
Fixed thusly:
The clone is The clone is specified by NAME, using the following name
mangling conventions
>> + 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.
You mean this should go in instead of 'These are defined in ...', right?
>> + 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".
Thanks
>> +GCC_METHOD5 (gcc_type, new_template_typename_parm,
> s/typename/type/
The purpose behind this choice was to avoid the unwanted "template type"
grouping. I figured "typename" would convey the correct concept, even
before the "new_template_*_parm" pattern could be formed. Hmm... I
wonder if I should have made it new_*_template_parm instead...
>> +GCC_METHOD2 (gcc_type, new_dependent_typespec,
> "typespec" usually means type-specifier.
Oops ;-)
> How about new_dependent_type_template_id?
Or new_dependent[_template]_specialization? I think I'd rather use the
term 'specialization' because I've used it in other primitives and in
the documentation, whereas it would be the only use of 'id'.
>> +GCC_METHOD5 (gcc_expr, new_dependent_value_expr,
> I don't think you need "value" here. Nor in the other *_value_expr names.
That was to distinguish it from type expressions. I was concerned it
could be misused to construct
>> +GCC_METHOD3 (gcc_expr, type_value_expr,
> Maybe "cast_expr"?
Works for me.
>> +/* 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.
The reason they ended up in the same spot is that I grouped the various
kinds of expressions according to the meta-types of the operands, so all
of the expressions that took a list of expressions (values), with or
without a type, ended up in the same API entry point.
>> +GCC_METHOD4 (gcc_expr, alloc_expr,
> new_expr?
new_ would be confusing, for it was used as a prefix for entry points
that introduce new classes, new fields, etc.
> 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?
The 'build_' ones were inherited from the libcc1 API for C. The major
API departure from C's build_decl made me rename it to new_decl, and
then I followed the new_ pattern when introducing other names. IOW, no
strong reasons, just ones that offer insights into the development
history of the API. I guess we could change them all to build_, and use
new_ for alloc_expr, but I'm hesitant, because of the inconvenience this
might cause to the GDB folks that may have already got used to this
admittedly inconsistent convention.
>> +/* 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?
The class declaration; start_class_definition takes the decl returned by
new_decl and starts its definition.
>> + 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.
Hmm, I think it ended up meaning the opposite of what I meant, after
some changes to the sentence and elsewhere. If the lambda is not parm-,
field- or nsvar-scoped, then it must be local-scoped, i.e., the scope needs
TO be a function. I'm changing it to 'must'.
I realize now that we'd never look up or have reasons to define a
nsvar-scoped closure type (from a lambda used in the initializer of a
namespace-scoped variable), but if we're debugging the lambda function
or a member of a nested type thereof, then we would want to define the
closure type. The code in place will support that AFAICT, but I'm not
sure I'd thought of that.
>> +/* 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.
Will do. set_deferred_function_default_args was a late addition, when I
realized some cases required it, and I was undecided between keeping the
machinery that had been introduced before or removing it. So removing
it is ;-)
>> +/* FIXME: do we need to support argument packs? */
> You shouldn't, they're an internal detail.
Thanks for the confirmation.
>> + /* 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?
Not the last sentence, thanks. The comment now says:
/* We intentionally cannot express inline, constexpr, or virtual
override for functions. We can't inline or constexpr-replace
without a source-level body. The override keyword is only
meaningful within the definition of the containing class. */
--
Alexandre Oliva, freedom fighter http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/ FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer
