fix a typo in comment of AddConversionCandidate

[zhouyizhou]

Summary

Hi
I think the comment above the condition expression ICS.Standard.First == ICK_Lvalue_To_Rvalue should be:

In the second case, if the reference is an rvalue reference and
the first standard conversion sequence of the user-defined
conversion sequence includes an lvalue-to-rvalue conversion, the
program is ill-formed.

Many thanks
Zhouyi Zhou <[e-mail]>

[aaron.ballman]

The existing comment is correct according to my copy of the C++11 standard, but the standard has changed since C++11 and those words no longer appear in http://eel.is/c++draft/dcl.init.ref#5. Some more investigation is needed, but I suspect the correct action to take here is to update the comment based on the current standards wording or to see if there's a bug because the code does not match the comment.

[zhouyizhou]

In D133066#3764384, @aaron.ballman wrote:

The existing comment is correct according to my copy of the C++11 standard, but the standard has changed since C++11 and those words no longer appear in http://eel.is/c++draft/dcl.init.ref#5. Some more investigation is needed, but I suspect the correct action to take here is to update the comment based on the current standards wording or to see if there's a bug because the code does not match the comment.

thank Aaron for review my patch,
I am a passionate beginner,
this is a very good learning experience for me ;-) I am looking forward to seeing the final change.

Thanks again

[aaron.ballman]

In D133066#3765503, @zhouyizhou wrote:

In D133066#3764384, @aaron.ballman wrote:

The existing comment is correct according to my copy of the C++11 standard, but the standard has changed since C++11 and those words no longer appear in http://eel.is/c++draft/dcl.init.ref#5. Some more investigation is needed, but I suspect the correct action to take here is to update the comment based on the current standards wording or to see if there's a bug because the code does not match the comment.

thank Aaron for review my patch,
I am a passionate beginner,

Welcome to the community, we're glad to have you here!

this is a very good learning experience for me ;-) I am looking forward to seeing the final change.

Happy to help, but to be clear on the next steps: are you planning to do the investigation work, or were you hoping someone else would do it?

[zhouyizhou]

In D133066#3767146, @aaron.ballman wrote:

In D133066#3765503, @zhouyizhou wrote:

In D133066#3764384, @aaron.ballman wrote:

The existing comment is correct according to my copy of the C++11 standard, but the standard has changed since C++11 and those words no longer appear in http://eel.is/c++draft/dcl.init.ref#5. Some more investigation is needed, but I suspect the correct action to take here is to update the comment based on the current standards wording or to see if there's a bug because the code does not match the comment.

thank Aaron for review my patch,
I am a passionate beginner,

Welcome to the community, we're glad to have you here!

Thank Aaron for your encouragement and guidance! Hope I can be some beneficial to the community.

this is a very good learning experience for me ;-) I am looking forward to seeing the final change.

Happy to help, but to be clear on the next steps: are you planning to do the investigation work, or were you hoping someone else would do it?

As an amateur, this is a difficult job for me, but I can't help taking a try.

Following your guidance, I found the original C++11 document on the internet:
https://raw.githubusercontent.com/yjlintw/book-Coding-Developer/master/%E6%A0%87%E5%87%86%E6%96%87%E6%A1%A3/ISO%20IEC%2014882%202011%20(C%2B%2B11).pdf
(the non ASCII code in URL means this document is maintained by a Chinese like me).
And those words are there!

I am eager to do some investigation work in the elementary stage, but I believe the final work should be done by someone else.

Thanks again for your enthusiasm and your patience!

Cheers
Zhouyi

[aaron.ballman]

In D133066#3768091, @zhouyizhou wrote:

In D133066#3767146, @aaron.ballman wrote:

In D133066#3765503, @zhouyizhou wrote:

In D133066#3764384, @aaron.ballman wrote:

The existing comment is correct according to my copy of the C++11 standard, but the standard has changed since C++11 and those words no longer appear in http://eel.is/c++draft/dcl.init.ref#5. Some more investigation is needed, but I suspect the correct action to take here is to update the comment based on the current standards wording or to see if there's a bug because the code does not match the comment.

thank Aaron for review my patch,
I am a passionate beginner,

Welcome to the community, we're glad to have you here!

Thank Aaron for your encouragement and guidance! Hope I can be some beneficial to the community.

this is a very good learning experience for me ;-) I am looking forward to seeing the final change.

Happy to help, but to be clear on the next steps: are you planning to do the investigation work, or were you hoping someone else would do it?

As an amateur, this is a difficult job for me, but I can't help taking a try.

For what it's worth, it's not easy for experts either. :-D

Following your guidance, I found the original C++11 document on the internet:
https://raw.githubusercontent.com/yjlintw/book-Coding-Developer/master/%E6%A0%87%E5%87%86%E6%96%87%E6%A1%A3/ISO%20IEC%2014882%202011%20(C%2B%2B11).pdf
(the non ASCII code in URL means this document is maintained by a Chinese like me).
And those words are there!

Yup, that matches my copy of C++11; btw, you can find a late-stage working draft on the committee website at: https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2011/n3242.pdf

I am eager to do some investigation work in the elementary stage, but I believe the final work should be done by someone else.

Thanks again for your enthusiasm and your patience!

My pleasure!

For the moment, I don't think any changes are needed here regarding the comment.

[zhouyizhou]

In D133066#3780900, @aaron.ballman wrote:

In D133066#3768091, @zhouyizhou wrote:

In D133066#3767146, @aaron.ballman wrote:

In D133066#3765503, @zhouyizhou wrote:

In D133066#3764384, @aaron.ballman wrote:

The existing comment is correct according to my copy of the C++11 standard, but the standard has changed since C++11 and those words no longer appear in http://eel.is/c++draft/dcl.init.ref#5. Some more investigation is needed, but I suspect the correct action to take here is to update the comment based on the current standards wording or to see if there's a bug because the code does not match the comment.

thank Aaron for review my patch,
I am a passionate beginner,

Welcome to the community, we're glad to have you here!

Thank Aaron for your encouragement and guidance! Hope I can be some beneficial to the community.

this is a very good learning experience for me ;-) I am looking forward to seeing the final change.

Happy to help, but to be clear on the next steps: are you planning to do the investigation work, or were you hoping someone else would do it?

As an amateur, this is a difficult job for me, but I can't help taking a try.

For what it's worth, it's not easy for experts either. :-D

Thanks again for your patience, and sorry for the late feedback, I think I will spend 3 more weeks to work out and submit my tentative work.

Following your guidance, I found the original C++11 document on the internet:
https://raw.githubusercontent.com/yjlintw/book-Coding-Developer/master/%E6%A0%87%E5%87%86%E6%96%87%E6%A1%A3/ISO%20IEC%2014882%202011%20(C%2B%2B11).pdf
(the non ASCII code in URL means this document is maintained by a Chinese like me).
And those words are there!

Yup, that matches my copy of C++11; btw, you can find a late-stage working draft on the committee website at: https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2011/n3242.pdf

Yes, and I also find those words in above document and the example that follows is almost the same except a minor difference.

I am eager to do some investigation work in the elementary stage, but I believe the final work should be done by someone else.

Thanks again for your enthusiasm and your patience!

My pleasure!

For the moment, I don't think any changes are needed here regarding the comment.

Thanks again for your guidance. OK. and I use "git log -p cba72b1f620fd" the study the original submit which add those words.
This is a fruitful and happy journey for me ;-)

Cheers
Zhouyi

[zhouyizhou]

my current progress - succesfully build commit cba72b1f620fd on debian6:

1. git checkout cba72b1f620fd
2. download debian-6.0.10-amd64-DVD-1.iso and use it to create a debian6 virtual machine
3. debian6/llvm-project>cp -fr clang llvm/tools
4. modifiy llvm/tools/CMakefileLists.txt:

diff --git a/llvm/tools/CMakeLists.txt b/llvm/tools/CMakeLists.txt
index 2f37911..71cdbcc 100644

• a/llvm/tools/CMakeLists.txt

+++ b/llvm/tools/CMakeLists.txt
@@ -46,8 +46,6 @@ add_subdirectory(llvm-stub)
add_subdirectory(edis)
add_subdirectory(llvmc)

-if( EXISTS ${CMAKE_CURRENT_SOURCE_DIR}/clang/CMakeLists.txt )

• add_subdirectory( ${CMAKE_CURRENT_SOURCE_DIR}/clang )

-endif( EXISTS ${CMAKE_CURRENT_SOURCE_DIR}/clang/CMakeLists.txt )
+add_subdirectory(clang)

5. debian6/llvm-project>mkdir build
6. debian6/llvm-project>cd build
7. cmake ../llvm -DLLVM_TARGETS_TO_BUILD="X86" -DCMAKE_BUILD_TYPE="Debug" -DCMAKE_C_COMPILER=gcc -DCMAKE_CXX_COMPILER=g++
8. make clang -j 8

I am now begin to debugging newly build clang-2.9

Thanks again for your guidance and encouragement ;-)
cheers
Zhouyi

[zhouyizhou]

After 4 weeks' study, I think the comment didn't need to be changed, sorry to have bring your so much trouble.
During this valuable process of studying, I grow up a lot. I learned to read the C++ standard, and compare the standard to its implementation.
In my case, the "user-defined conversion" is the variable "Candidate", the "second standard conversion sequence" is the object member "Candidate.FinalConversion".
The only pity during my study is that I can't find a example code to let Clang (even with commit cba72b1f620fd) hit the code below above comment.

I am going to close this thread.

Thanks for reviewing my patch, and for your guidance.
Cheers
Zhouyi

[aaron.ballman]

In D133066#3845136, @zhouyizhou wrote:

After 4 weeks' study, I think the comment didn't need to be changed, sorry to have bring your so much trouble.

No worries at all, there was no trouble here!

During this valuable process of studying, I grow up a lot. I learned to read the C++ standard, and compare the standard to its implementation.
In my case, the "user-defined conversion" is the variable "Candidate", the "second standard conversion sequence" is the object member "Candidate.FinalConversion".
The only pity during my study is that I can't find a example code to let Clang (even with commit cba72b1f620fd) hit the code below above comment.

I'm glad you found it valuable! As for a code example to hit the code below that comment, the closest I could come is:

struct S {
  operator int&& () const { return 12; }
};

void func(int &&i);

int main() {
  S s;
  func(s);
}

however, that still fails the lvalue-to-rvalue test. I poked at it for a while and I'm not seeing a case where it's possible to hit that condition (it doesn't mean one doesn't exist, just that I didn't have the time to chase it down fully).

[zhouyizhou]

In D133066#3846907, @aaron.ballman wrote:

In D133066#3845136, @zhouyizhou wrote:

After 4 weeks' study, I think the comment didn't need to be changed, sorry to have bring your so much trouble.

No worries at all, there was no trouble here!

Thank Aaron for your patience and for your encouragement!

During this valuable process of studying, I grow up a lot. I learned to read the C++ standard, and compare the standard to its implementation.
In my case, the "user-defined conversion" is the variable "Candidate", the "second standard conversion sequence" is the object member "Candidate.FinalConversion".
The only pity during my study is that I can't find a example code to let Clang (even with commit cba72b1f620fd) hit the code below above comment.

I'm glad you found it valuable! As for a code example to hit the code below that comment, the closest I could come is:

struct S {
  operator int&& () const { return 12; }
};

void func(int &&i);

int main() {
  S s;
  func(s);
}

however, that still fails the lvalue-to-rvalue test. I poked at it for a while and I'm not seeing a case where it's possible to hit that condition (it doesn't mean one doesn't exist, just that I didn't have the time to chase it down fully).

Yes! this is the closest example that I can try to hit the code below that comment. Yes, I can't hit that condition both with "commit cba72b1f620fd" (I debug her in old debian 6 virtual machine) and clang current. However this is still a very fruitful journal for me ;-)

I am going to close this thread after a couple of days.

And thanks again for your time ;-)

Cheers
Zhouyi

clang/lib/Sema/SemaOverload.cpp

if (Conversion->getPrimaryTemplate() &&
         GetConversionRank(ICS.Standard.Second) != ICR_Exact_Match) {
       Candidate.Viable = false;
       Candidate.FailureKind = ovl_fail_final_conversion_not_exact;
       return;
     }
 
     // C++0x [dcl.init.ref]p5:
     //    In the second case, if the reference is an rvalue reference and
-    //    the second standard conversion sequence of the user-defined
+    //    the first standard conversion sequence of the user-defined
     //    conversion sequence includes an lvalue-to-rvalue conversion, the
     //    program is ill-formed.
     if (ToType->isRValueReferenceType() &&
         ICS.Standard.First == ICK_Lvalue_To_Rvalue) {
       Candidate.Viable = false;
       Candidate.FailureKind = ovl_fail_bad_final_conversion;
       return;
     }