Trying to work on "C++ Type-checking for copy assignment" project

Hello,
I am interested to provide some help to your project. So I made some
changes for subj, as assignment resolution does not work corectly for
C++. Work is not finished, but I want to check if I am on the right
direction. Please check attached patch for clang. I changed
Sema::CreateOverloadedBinOp so it does not use built-in assignment
operator in case left operand is a class instance but generates error
err_ovl_no_viable_oper ("no viable overloaded '='") instead. Is it
proper error message for this case?
I added copy-assignment.cpp to validate this change, but the change
made other tests fail (error message changed from incompatible type
assigning '%0', expected '%1' to no viable overloaded '='), I can fix
them but I need to know what error should be used in this case.

Thanks,
Vyacheslav

patch (2.57 KB)

Vyacheslav Kononenko wrote:

Hello,
I am interested to provide some help to your project.

Great :slight_smile:

So I made some
changes for subj, as assignment resolution does not work corectly for
C++. Work is not finished, but I want to check if I am on the right
direction.

There may be a deeper issue here. Why is falling back to the built-in
assignment operator the wrong thing to do? Apparently it does something
that lets invalid cases pass. Why is that so?

Please check attached patch for clang. I changed
Sema::CreateOverloadedBinOp so it does not use built-in assignment
operator in case left operand is a class instance but generates error
err_ovl_no_viable_oper ("no viable overloaded '='") instead. Is it
proper error message for this case?
  

This is a tricky issue. For some cases, "no viable overloaded '='" is
the better message (e.g. for the nb = constB testcase). For others,
especially when there are no user-defined assignment operators in the
target type, 'incompatible type' would be the better message.

What does it say for this:

int i = hasNoConversionToInt();

For this case, no viable overload would definitely be the wrong message.

Sebastian

There may be a deeper issue here. Why is falling back to the built-in
assignment operator the wrong thing to do? Apparently it does something
that lets invalid cases pass. Why is that so?

If I understand correctly in C++ assignment to a class instance can be
done only by overloaded operator= implicit or explicit. So I think
case with assignment operator and class instance on the left could be
done only by overload resolution in general.

For others,
especially when there are no user-defined assignment operators in the
target type, 'incompatible type' would be the better message.

There is always assignment operator in a class - implicit one. So no
viable operator= found probably is correct message even in this case.
Maybe it should print additional diagnostics - which assignment
operators are available (like it does for ambiguity) but that is
different question.

What does it say for this:

int i = hasNoConversionToInt();

For this case, no viable overload would definitely be the wrong message.

Changes that I made only for the case when class instance on the left.
This one will work as before. I can actually put this as a test case
there as well.

Thanks,
Vyacheslav

Updated copy-assignment.cpp is attached

copy-assignment.cpp (1.57 KB)

Vyacheslav Kononenko wrote:

There may be a deeper issue here. Why is falling back to the built-in
assignment operator the wrong thing to do? Apparently it does something
that lets invalid cases pass. Why is that so?
    

If I understand correctly in C++ assignment to a class instance can be
done only by overloaded operator= implicit or explicit. So I think
case with assignment operator and class instance on the left could be
done only by overload resolution in general.

OK, that makes sense.

For others,
especially when there are no user-defined assignment operators in the
target type, 'incompatible type' would be the better message.
    

There is always assignment operator in a class - implicit one. So no
viable operator= found probably is correct message even in this case.
  

But implicit assignment operators are not user-defined. I just wonder
how hard we should try in making POD types feel like built-ins.

Maybe it should print additional diagnostics - which assignment
operators are available (like it does for ambiguity) but that is
different question.

I'm not sure, but I don't think our lookup automatically limits it to
those operators that are viable for the left-hand side, so we'd have to
filter all others out manually. We certainly don't want every possible
assignment operator in that list.
Now, with assignment only being overloadable within the class, it
probably wouldn't be that bad, but I don't want to rely on that.

What does it say for this:

int i = hasNoConversionToInt();

For this case, no viable overload would definitely be the wrong message.
    

Changes that I made only for the case when class instance on the left.
This one will work as before. I can actually put this as a test case
there as well.
  

Please do, and update the old test cases, and resend the patch. I'll
commit it. We can do minor improvements later.

Sebastian

But implicit assignment operators are not user-defined. I just wonder
how hard we should try in making POD types feel like built-ins.

You mean by behavior or error message should be the same as for
builtin? I think implicit assignment operator should provide the same
behavior.

I'm not sure, but I don't think our lookup automatically limits it to
those operators that are viable for the left-hand side, so we'd have to
filter all others out manually. We certainly don't want every possible
assignment operator in that list.

No, of course not. Only assignment operators from the class and
parents. Yes that will require filtering the list. Another idea is to
print "incompatible type" message when there is only one assignment
operator available. That will require filtering that list as well.

Please do, and update the old test cases, and resend the patch. I'll
commit it. We can do minor improvements later.

New patch is attached. I also added some test cases for compound
assignment operator.

Thanks,
Vyacheslav

patch (4.58 KB)

I may have missed it in the discussion so far - apologies if I did.

The discussion so far seems to have left out the fact that if the
programmer doesn’t specify a user-defined assignement operator for
class type T, then the compiler is supposed to generate an assginment
operator. This compiler-generated assignment operator member function
then does a bitwise copy of the member data.

[ It is up to the programmer for class T to know when the compiler-generated
assignment operator will be sufficient or not - and if they choose to use
the compiler-generated when it’s not safe then they can get incorrectly
behaving code. OTOH, if they choose to always write their own, then they
may be bloating their code and slowing down fundamental operations. ]

Again, if this part of the spec wasn’t left out (or forgotten), then apologies
for the wasted bandwith.

Brian

Sebastian,
Is there a problem with my patch? Iw ant to continue to work on this
but to have too many unsubmitted changes is uncomfortable.

Thanks,
Vyacheslav

Vyacheslav Kononenko wrote:

Sebastian,
Is there a problem with my patch? Iw ant to continue to work on this
but to have too many unsubmitted changes is uncomfortable.
  

I'm sorry, but I've been very busy these last days. I have a long
weekend coming up, so I'll be able to look at it in detail and perhaps
commit it.

Sebastian

Vyacheslav Kononenko wrote:

New patch is attached. I also added some test cases for compound
assignment operator.
  

Thanks a lot. I've committed this here:
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20090518/017525.html

Sebastian