ArrayType and qualifiers

In recent clang versions while syntactically parsed ArrayTypes have
qualifiers in element type (congruently with the standard), canonical
ArrayTypes have qualifers *only* on array type.

I have two questions:

1) why element qualifiers are *moved* to ArrayType instead of to be
*propagated* to ArrayType? (i.e. qualifiers would be in both ArrayType
and element type)

2) considered that now calling getElementType() on a canonical ArrayType
returns the wrong result, how is possible to get the right element type
e.g. using TypeVisitor?

P.S. The way described in 1) would permit to have an AST congruent with
standard (i.e. the qualifiers in array would be a clang only thing and
the qualifiers in element type would be there to ensure standard
adherence). Of course it would solve also point 2) :wink:

In recent clang versions while syntactically parsed ArrayTypes have
qualifiers in element type (congruently with the standard), canonical
ArrayTypes have qualifers *only* on array type.

Syntactically parsed array types can have qualifiers at any level due to
typedefs, just like always.

1) why element qualifiers are *moved* to ArrayType instead of to be
*propagated* to ArrayType? (i.e. qualifiers would be in both ArrayType
and element type)

Because it's still quite expensive to canonicalize a type in that representation,
and it becomes very awkward to manipulate qualifiers on even canonical
types.

2) considered that now calling getElementType() on a canonical ArrayType
returns the wrong result, how is possible to get the right element type
e.g. using TypeVisitor?

I hadn't really considered the effects this would have on people using
TypeVisitor on a canonical type. Mostly, I wasn't aware that that was at all
common. If it is, maybe we should change TypeVisitor.

John.

In recent clang versions while syntactically parsed ArrayTypes have
qualifiers in element type (congruently with the standard), canonical
ArrayTypes have qualifers *only* on array type.

Syntactically parsed array types can have qualifiers at any level due to
typedefs, just like always.

Sorry, I should have explained better myself: I meant qualifier on
syntactically expressed ArrayTypes, not typedefs that when desugared are
array types.

1) why element qualifiers are *moved* to ArrayType instead of to be
*propagated* to ArrayType? (i.e. qualifiers would be in both ArrayType
and element type)

Because it's still quite expensive to canonicalize a type in that representation,
and it becomes very awkward to manipulate qualifiers on even canonical
types.

I'm not sure to follow you: canonicalization would not become more CPU
expensive with that representation, we've tried the patch and it is trivial.

OTOH the patch lead to some failure with tests similar to this:

void f() {
  const int (*x)[1] = 0;
  int (*y)[1] = 0;
  x = y;
}

probably due to the fact that type convertibility check currently does
not expect to find a difference also on int qualifiers.

Note that however also with current svn trunk we have a problem with
this test: the assignment x = y should be compiled without warning in
C++ *but* it's an assignment between incompatible pointers in C (clang
currently does not emit any warning on both C and C++).

I've not verified if this behaviour is a regression introduced by recent
changes.

2) considered that now calling getElementType() on a canonical ArrayType
returns the wrong result, how is possible to get the right element type
e.g. using TypeVisitor?

I hadn't really considered the effects this would have on people using
TypeVisitor on a canonical type. Mostly, I wasn't aware that that was at all
common. If it is, maybe we should change TypeVisitor.

I don't understand how this might be feasible: we enter
TypeVisitor::Visit with a Type* and as a consequence in the ArrayType
dispatcher we have already lost any info about qualifier that should be
applied to element type. The fact is that currently a canonical
ArrayType has not enough info to know the qualifiers of its element, I
believe this is an unfortunate situation.

Note also that to increase further the confusion, the API has also
QualType ASTContext::getBaseElementType(const ArrayType *VAT) const...
it's a mystery for me how this might work correctly. I'd hope it is
never called :wink:

1) why element qualifiers are *moved* to ArrayType instead of to be
*propagated* to ArrayType? (i.e. qualifiers would be in both ArrayType
and element type)

Because it's still quite expensive to canonicalize a type in that representation,
and it becomes very awkward to manipulate qualifiers on even canonical
types.

I'm not sure to follow you: canonicalization would not become more CPU
expensive with that representation, we've tried the patch and it is trivial.

Right now, canonicalizing a type never requires rebuilding types, which it used to. Any patch which uses your proposed representation has to rebuild types when adding qualifiers to an array type, which is quite possible. If your trivial patch doesn't do this, it is broken.

Your patch probably assumes that we only ever add and remove qualifiers in ways that mirror the patterns of how they can be legally written in declarators, which is simply not correct.

void f() {
const int (*x)[1] = 0;
int (*y)[1] = 0;
x = y;
}

Note that however also with current svn trunk we have a problem with
this test: the assignment x = y should be compiled without warning in
C++ *but* it's an assignment between incompatible pointers in C (clang
currently does not emit any warning on both C and C++).

It's not an assignment between incompatible pointers in C. y is a T*, where T = int[1]; converting that to the type of x (const T*) is a 6.3.2.3p2 pointer conversion.

You are probably thinking of the C++ rule governing indirect pointer conversions (T** can be converted to T cv * const *), but that is not the rule in effect here.

2) considered that now calling getElementType() on a canonical ArrayType
returns the wrong result, how is possible to get the right element type
e.g. using TypeVisitor?

I hadn't really considered the effects this would have on people using
TypeVisitor on a canonical type. Mostly, I wasn't aware that that was at all
common. If it is, maybe we should change TypeVisitor.

I don't understand how this might be feasible: we enter
TypeVisitor::Visit with a Type* and as a consequence in the ArrayType
dispatcher we have already lost any info about qualifier that should be
applied to element type. The fact is that currently a canonical
ArrayType has not enough info to know the qualifiers of its element, I
believe this is an unfortunate situation.

This would obviously have to be done at the level of TypeVisitor::Visit(QualType).

That brings up a good point, actually — Visit(const Type*) is already stripping qualifiers off of every other type, so I'm not sure why I'm supposed to be particularly bothered that it strips the qualifiers off array types.

Note also that to increase further the confusion, the API has also
QualType ASTContext::getBaseElementType(const ArrayType *VAT) const...
it's a mystery for me how this might work correctly. I'd hope it is
never called :wink:

The expectation is that the user passes in something they acquired from ASTContext::getAsArrayType, which does this (non-canonical) pushing down of qualifiers.

John.

Note that however also with current svn trunk we have a problem with
this test: the assignment x = y should be compiled without warning in
C++ *but* it's an assignment between incompatible pointers in C (clang
currently does not emit any warning on both C and C++).

It's not an assignment between incompatible pointers in C. y is a T*, where T = int[1]; converting that to the type of x (const T*) is a 6.3.2.3p2 pointer conversion.

It seems you're definitely right.

The weird thing is that three independent implementation (GNU, Intel and
Comeau) get the same *wrong* result in C... I'm astonished, I wonder how
this might be happened.

returns the wrong result, how is possible to get the right element type
e.g. using TypeVisitor?

I hadn't really considered the effects this would have on people using
TypeVisitor on a canonical type. Mostly, I wasn't aware that that was at all
common. If it is, maybe we should change TypeVisitor.

I don't understand how this might be feasible: we enter
TypeVisitor::Visit with a Type* and as a consequence in the ArrayType
dispatcher we have already lost any info about qualifier that should be
applied to element type. The fact is that currently a canonical
ArrayType has not enough info to know the qualifiers of its element, I
believe this is an unfortunate situation.

This would obviously have to be done at the level of TypeVisitor::Visit(QualType).

That brings up a good point, actually — Visit(const Type*) is already stripping qualifiers off of every other type, so I'm not sure why I'm supposed to be particularly bothered that it strips the qualifiers off array types.

I believe the case is different here: for every other type the
qualifiers stripping is local and has no effect on inner types, for
arrays the qualifier stripping in array alter the interpretation of
inner nodes (I think that RecursiveASTVisitor will suffer of this).

However I think that we will proceed changing our visitors to save the
array qualifiers for later reapplication to element type.

Note that however also with current svn trunk we have a problem with

this test: the assignment x = y should be compiled without warning in

C++ but it’s an assignment between incompatible pointers in C (clang

currently does not emit any warning on both C and C++).

It’s not an assignment between incompatible pointers in C. y is a T*, where T = int[1]; converting that to the type of x (const T*) is a 6.3.2.3p2 pointer conversion.

It seems you’re definitely right.

Hmm. Actually, I think I’m wrong. :slight_smile: In C, array types are never qualified, only their element types; so while my argument is sound in principle, it’s not actually following the specification.

That said, this is quite arguably a flaw in the specification, and it’s been so argued in quite a few places, as mentioned and linked from here:
http://stackoverflow.com/questions/305293/constant-array-types-in-c-flaw-in-standard
so we should probably add a -pedantic warning for this at least. I’ve file http://llvm.org/bugs/show_bug.cgi?id=9082 to track this.

The weird thing is that three independent implementation (GNU, Intel and
Comeau) get the same wrong result in C… I’m astonished, I wonder how
this might be happened.

FWIW, Intel and Comeau aren’t independent implementations; they’re both EDG-based.

returns the wrong result, how is possible to get the right element type

e.g. using TypeVisitor?

I hadn’t really considered the effects this would have on people using

TypeVisitor on a canonical type. Mostly, I wasn’t aware that that was at all

common. If it is, maybe we should change TypeVisitor.

I don’t understand how this might be feasible: we enter

TypeVisitor::Visit with a Type* and as a consequence in the ArrayType

dispatcher we have already lost any info about qualifier that should be

applied to element type. The fact is that currently a canonical

ArrayType has not enough info to know the qualifiers of its element, I

believe this is an unfortunate situation.

This would obviously have to be done at the level of TypeVisitor::Visit(QualType).

That brings up a good point, actually — Visit(const Type*) is already stripping qualifiers off of every other type, so I’m not sure why I’m supposed to be particularly bothered that it strips the qualifiers off array types.

I believe the case is different here: for every other type the
qualifiers stripping is local and has no effect on inner types, for
arrays the qualifier stripping in array alter the interpretation of
inner nodes (I think that RecursiveASTVisitor will suffer of this).

That’s true.

However I think that we will proceed changing our visitors to save the
array qualifiers for later reapplication to element type.

Thank you. I’m sorry to keep changing the AST out from under you; we really do think this is the right technical design.

John.

Note that however also with current svn trunk we have a problem with
this test: the assignment x = y should be compiled without warning in
C++ *but* it's an assignment between incompatible pointers in C (clang
currently does not emit any warning on both C and C++).

It's not an assignment between incompatible pointers in C. y is a
T*, where T = int[1]; converting that to the type of x (const T*) is
a 6.3.2.3p2 pointer conversion.

It seems you're definitely right.

Hmm. Actually, I think I'm wrong. :slight_smile: In C, array types are never
qualified, only their element types; so while my argument is sound in
principle, it's not actually following the specification.

That said, this is quite arguably a flaw in the specification, and it's
been so argued in quite a few places, as mentioned and linked from here:
  http://stackoverflow.com/questions/305293/constant-array-types-in-c-flaw-in-standard
so we should probably add a -pedantic warning for this at least. I've
file http://llvm.org/bugs/show_bug.cgi?id=9082 to track this.

stackoverflow seems to refers to case of cast from pointer to int to
pointer of array of int, while the example I wrote is about

I don't see how 6.3.2.3p2 might be read differently from what you has
written in previous email.

Take this modified example:

void f() {
  const int (*x)[1] = 0;
  int (*y)[1] = 0;
  x = y;
}

typedef int Type[1];

void g() {
  const Type* x = 0;
  Type* y = 0;
  x = y;
}

6.3.2.3p2 says: For any qualifier q, a pointer to a non-q-qualified type
may be converted to a pointer to the q-qualified version of the type;
the values stored in the original and converted pointers shall compare
equal.

I really don't think that the standard C99 want to do an exception for a
specific instance of Type... (or at least I hope so :wink:

I believe the case is different here: for every other type the
qualifiers stripping is local and has no effect on inner types, for
arrays the qualifier stripping in array alter the interpretation of
inner nodes (I think that RecursiveASTVisitor will suffer of this).

That's true.

However I think that we will proceed changing our visitors to save the
array qualifiers for later reapplication to element type.

Thank you. I'm sorry to keep changing the AST out from under you; we
really do think this is the right technical design.

Don't be sorry, every good technical choice about AST is very welcome
for us and I've already adapted our code to restore qualifier position
where standard mandates, so everything is working again.

I've written to convince myself this was indeed a good technical choice
and to be honest I've still doubts for some techinical reasons:

1) incongruence on representations of canonical types and syntactical
type about position of qualifiers

2) the element type AST node has not the qualifiers that standard mandates

3) asimmetry about treatment of ComplexType and VectorType wrt ArrayType
(the qualifiers are where standard mandates in both canonical and syntactic)

4) fragile clang API, where we risk everytime to lose element qualifiers
descending on the type

Now that I'm sure you know our perspective I can stop to pester you :wink:

Please forget this part, I don't know what I had in mind when I wrote
this :wink:

Hmm. Actually, I think I'm wrong. :slight_smile: In C, array types are never
qualified, only their element types; so while my argument is sound in
principle, it's not actually following the specification.

That said, this is quite arguably a flaw in the specification, and it's
been so argued in quite a few places, as mentioned and linked from here:
http://stackoverflow.com/questions/305293/constant-array-types-in-c-flaw-in-standard
so we should probably add a -pedantic warning for this at least. I've
file http://llvm.org/bugs/show_bug.cgi?id=9082 to track this.

stackoverflow seems to refers to case of cast from pointer to int to
pointer of array of int, while the example I wrote is about

Right, that post on stackoverflow is about an explicit cast that gives a warning under GCC because it supposedly removes qualifiers, for the reason I give below. Since that diagnostic is supposed to be a QoI, the-programmer-is-doing-something-dangerous warning, GCC is wrong to produce it *regardless* of how the standard treats qualifiers, because the behavior in question is not actually unsafe. Put another way, the first responsibility of a diagnostic like that should be utility to programmers, not pedantry. That said, I'm sure it's just a lazy implementation.

I don't see how 6.3.2.3p2 might be read differently from what you has
written in previous email.

Take this modified example:

void f() {
const int (*x)[1] = 0;
int (*y)[1] = 0;
x = y;
}

typedef int Type[1];

void g() {
const Type* x = 0;
Type* y = 0;
x = y;
}

6.3.2.3p2 says: For any qualifier q, a pointer to a non-q-qualified type
may be converted to a pointer to the q-qualified version of the type;
the values stored in the original and converted pointers shall compare
equal.

The question is whether the const-qualified version of int[1] is const int[1], or whether it's the formally-impossible type int[1] const. I think the most likely interpretation is the latter, which would prohibit this conversion. Basically, the standard follows clang's old canonical type representation, except that qualifier queries aren't supposed to look through array types.

I think that's incontrovertibly wrong as an abstract point of language design, so I have no problems with the fact that we're Doing The Right Thing (tm) instead, but we probably should have a compatibility / pedantic warning about it, which is why I filed PR9082.

Don't be sorry, every good technical choice about AST is very welcome
for us and I've already adapted our code to restore qualifier position
where standard mandates, so everything is working again.

Glad to hear it wasn't too difficult.

I've written to convince myself this was indeed a good technical choice
and to be honest I've still doubts for some techinical reasons:

1) incongruence on representations of canonical types and syntactical
type about position of qualifiers

Well, this was true before in the general case, but I see your point.

2) the element type AST node has not the qualifiers that standard mandates

I'm okay with having a slightly different representation from the standard in this case.

4) fragile clang API, where we risk everytime to lose element qualifiers
descending on the type

This is already true of the most common case, i.e. clients that walk non-canonical types, so unfortunately everyone just has to know that array types are special. Once that's true, I think be able to add and remove qualifiers easily on canonical types is really a blessing.

John.