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

On 8/18/26 7:15 AM, Arsen Arsenović wrote:
> Jason Merrill <[email protected]> writes:
> 
>>> -      && 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 (...)'.

It actually wouldn't; in boolean contexts like conditions and && the 
expression is "contextually converted to bool" 
(https://eel.is/c++draft/conv.general#4), which uses an explicit 
conversion operator.

> 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.

It seems useful; wanting the cv-unqualified type comes up a lot.

>>> +  /* 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.

Fair enough; please add it to the comment, then.

>>> +  /* 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).

Sure, but doesn't that same argument apply to the second paragraph as it 
is?  In cases where there isn't a standard conversion, it's specifically 
because such a conversion would not be safe.

And we're only dealing with one set of qualifiers here, we could frame 
it in terms of whether cv1 int* can convert to cv2 int* without 
referring to an arbitrary type 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.

And yet you call the parameter "SUBSET" :)

Maybe "compatible_with" (in a sense similar to C++ reference-compatible)?

>> 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.
> 
>>> +/* 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.

Please; it's easy for me to jump to the definition of a symbol, but to 
find the declaration I need to do a text search.

Jason