Re: [PATCH gcc/* 2/2] gcc: stop using 'int' to represent sets of qualifiers

Arsen Arsenović <[email protected]> Tue, 18 Aug 2026 13:15:03 +0200
Newsgroups	gmane.comp.gcc.patches
Organization	BayLibre
Message-ID	<[email protected]>
Jason Merrill <[email protected]> writes:

> Thanks, and thanks for the pings.
>
> There are several git gcc-style issues that I won't repeat here, most of which
> seem like true positives.
>
>> 	(qualifier_set::qualifier_set): New.  Constructs a qualifier set
>> 	from a TREE_NODE.  Used where new expansion of TYPE_QUALS (which
>> 	contains a comma) breaks other macros.  :-(
>
> git gcc-verify rejects the emoticon as an unmatched open paren.
>
> But couldn't you avoid this problem by using qualifier_set (...) in TYPE_QUALS
> instead of {...}?

Ah, true; even just adding (...) around the contents of {} should
suffice.

>> -	  && TYPE_QUALS (TREE_TYPE (selector_type)) != TYPE_UNQUALIFIED)
>> +	  && TYPE_QUALS (TREE_TYPE (selector_type)) != qualifier_set {})
>
> Are all the changes like this necessary?  Don't these two have the
> same result?

No, they're not.

Originally, it was not possible to compare a qualifier_set with a
cv_qualifier (because there was no implicit conversion from cv_qualifier
to qualifier_set), so they'd have been.

This changed at some point, but apparently some places survived the
cleanup.

>> -      && TYPE_QUALS (element_type))
>> +      && TYPE_QUALS (element_type) != qualifier_set {})
>
> And how about an explicit operator bool so we don't need these changes?

I seem to recall that adding an implicit conversion operator would've
changed the meaning of some existing code, but I am not 100% confident
that my memory is right.  (That certainly happened when I added an
operator& as a shorthand for 'has'; I may be confusing those two)

I'll see if I can find any issue with adding an implicit conversion
operator.

Adding an explicit one would just mean needing to wrap these places in
'bool (...)'.

Or, we could settle on using 'nonempty_p' for all these cases.

>> -    unqual_elt = c_build_qualified_type (elt, KEEP_QUAL_ADDR_SPACE (quals));
>> +    unqual_elt = c_build_qualified_type (elt,
>> +					 quals.without (TYPE_QUAL_ALL));
> The quals.without (TYPE_QUAL_ALL) pattern seems awkward, especially given the
> semi-ambiguity of "ALL".  How about a without_cv member function?

Yes, "ALL" is ambiguous.  I couldn't think of a better term.

This only occurred once, which is why I didn't add 'without_cv'.

But, I can add it.

>> +  /* Construct an empty qualifier set with the generic address space.  Such a
>> +     qualifier set corresponds to unqualified types.  */
>> +  qualifier_set () = default;
>> +  static_assert (cv_qualifier {} == TYPE_UNQUALIFIED
>> +		 && addr_space_t {} == ADDR_SPACE_GENERIC,
>> +		 "We want the trivial default constructor to use those vals");
>
> Is there a reason we need the default constructor to be trivial rather than
> explicitly setting those values?  If so that should be documented in the
> comment.

I did that originally because of the unions inside c_declarator and
cp_declarator.

Those get used in both FEs as well as libcc1 and initialized via memset.

That could all be fixed up, but it ended up being an additional large
change when I tried to do so.

>> +  /* Returns true if qualifiers in SUBSET can be replaced with qualifiers in
>> +     THIS safely.
>> +
>> +     In general, this means that an object qualified per SUBSET can be used as
>> +     if it was qualified per this qualifier set (e.g. 'T' as 'const T', or
>> +     'const T' as 'const volatile AS1 T', presuming that AS1 is a superset of
>> +     the generic address space).
>> +
>> +     If NOP_ONLY, return 'true' iff a pointer with a pointee qualified via
>> +     SUBSET can be converted into a pointer with a pointee qualified via THIS
>> +     (i.e. if a NOP_EXPR conversion would be valid).  In particular, this means
>> +     address space mismatches are forbidden.  */
>> +  bool can_qualify (qualifier_set subset, bool nop_only = false) const;
>
> This name and the first paragraph are confusing.  What does "safely" mean?  If
> it means there's a C++ standard conversion, let's say that.

That's what the second paragraph expands on.

I didn't refer to standard conversions because a single qualifier set
alone doesn't suffice to decide whether a conversion is allowed (whether
'T * cv *' can be converted to 'const T * cv *' depends on 'cv' rather
than just the qualification of the innermost T).

> How about "superset_of"?

Hmm, a superset does not necessarily mean that THIS can be used to
qualify an object previously qualified as SUBSET (particularly when
NOP_ONLY, or when the sets differ by _Atomic qualification), so it has
different semantics.

> Also "nop_only" seems like it should be called pointee or
> (not_)toplevel or some such that describes the semantics rather than
> the representation.

Yeah, 'pointee' could work.

The semantics that I was going for is that "if not NOP_ONLY, then
replacing SUBSET with *this might require some work other than
reinterpretation; if NOP_ONLY, then reinterpretation is safe" which is,
indeed, phrased in terms of representation.

I can't think of any case other than pointee types where the compiler
can't insert the correct conversion sequence (and where it, thus,
requires reinterpretation), so that name may be okay.

>>  static bool
>> -cp_check_qualified_type (const_tree cand, const_tree base, int type_quals,
>> -			 cp_ref_qualifier rqual, tree raises, bool late)
>> +cp_check_qualified_type (const_tree cand, const_tree base,
>> +			 cv_qualifier type_quals, cp_ref_qualifier rqual,
>> +			 tree raises, bool late)
>>  {
>>    return (TYPE_QUALS (cand) == type_quals
>
> Should this be TYPE_QUALS_NO_ADDR_SPACE?

The C++ named address spaces patch would just revert that if it were.

And if, by some miracle, an address-space qualified type made it into
the C++ FE before the named address spaces patch lands, then this
function could say that CAND, which has an address space qualifier, is
equivalent to BASE without any address space qualification.

So, I think TYPE_QUALS is correct here, or at least it does not harm.
Certainly this must be TYPE_QUALS once NAS support lands.

>> +/* Documented next to declaration in tree.h.  */
>
> In GCC we generally document by the definition.

Yes, but I thought it'd be awkward to outline the documentation for just
two members (given that the rest are just inlined in the struct body),
so I figured that leaving the documentation next to the declaration and
a note here would work.

No strong feeling either way, I can move the documentation above the
definition, too.

>> -extern int cp_type_quals			(const_tree);
>> -extern int type_memfn_quals			(const_tree);
>> +extern cv_qualifier cp_type_quals		(const_tree);
>> +extern cv_qualifier type_memfn_quals		(const_tree);
>
> What's the rationale for the cp_type_quals having a different return type from
> TYPE_QUALS?

If cp_type_quals returned a qualifier_set, the C++ FE would be forced to
handle address space qualification.

Frontends that do not support address space qualification (like, in this
patch series, the C++ one) can implement their own ..._type_quals helper
that returns cv_qualifier and, as a result, they can ignore the
existence of address space qualification.

If they change their mind later, they can just change the return type of
..._type_quals and fix the errors that result from that.

In frontends that do either use TYPE_QUALS or have their own
..._type_quals that return qualifier_set OTOH, the compiler will make
sure that they handle address space qualification where they're handling
qualifiers.

Indeed, as an example, the C++ Named Address Space support v5 [1] was
implemented by just changing cp_type_quals to return qualifier_set, and
fixing the resulting compiler diagnostics.

>> +++ b/gcc/cp/cp-objcp-common.h
>> @@ -142,7 +142,10 @@ static const scoped_attribute_specs *const cp_objcp_attribute_table[] =
>>  #undef LANG_HOOKS_TREE_DUMP_DUMP_TREE_FN
>>  #define LANG_HOOKS_TREE_DUMP_DUMP_TREE_FN cp_dump_tree
>>  #undef LANG_HOOKS_TREE_DUMP_TYPE_QUALS_FN
>> -#define LANG_HOOKS_TREE_DUMP_TYPE_QUALS_FN cp_type_quals
>> +inline qualifier_set
>> +cp_type_quals_as_set (const_tree type)
>> +{ return { cp_type_quals (type) }; }
>> +#define LANG_HOOKS_TREE_DUMP_TYPE_QUALS_FN cp_type_quals_as_set
>
> This function can be defined (not inline) in cp-objc-common.cc, rather than the
> header.

Ack, I'll move it there (but do note that it goes away when the named
address spaces patch gets merged).

[1] https://inbox.sourceware.org/gcc-patches/[email protected]/
-- 
Arsen Arsenović
signature.asc (application/pgp-signature, 430 B)