On Sun, Jan 22, 2017 at 6:06 PM, Alexandre Oliva <aol...@redhat.com> wrote:
> 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.
Well, in general TREE_LISTs are discouraged nowadays in favor of
hash_sets or VECs. If it's a single entry, putting it there alone is
fine too.
>>> + // 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.
Sounds good.
>>> +/* 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 ;-)
Heh. But I'd prefer to unify them in the other direction; use of
"ansi" is obsolete.
>>> + 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.
Well, it's a property of the namespace. But I guess I won't insist on 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?
>
> 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.
Fair enough.
>>> + 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.
OK.
>>> +/* 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 ;-)
Please.
>>> +/* 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.
Please.
>>> + 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.
Fair enough, but then why pass a decl? Passing scope and identifier
would seem cleaner if that's what you mean.
>>> 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?
I mean have the compiler recognize that the name is magic and infer
that it's a special function.
>>> +/* 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
Without the duplicate "The clone is", I hope? :)
>>> + 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?
Yes.
>>> +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...
Sure, that would be even better.
>>> +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'.
Well, a template-id is the syntactic term for combining a template
name and some template arguments, which seems like what this function
is doing. A template-id names a specialization, but isn't quite the
same.
>>> +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
I don't know what you mean by "type expressions".
>>> +/* 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.
Hmm, OK. Maybe expression_list_expr?
>>> +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.
Better to fix an inconsistent API now than leave it to confuse future
developers, I would think.
Jason
