[PATCH] D133066: fix a typo in comment of AddConversionCandidate

[zhouyizhou via Phabricator via cfe-commits]

zhouyizhou added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133066/new/

https://reviews.llvm.org/D133066

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133066: fix a typo in comment of AddConversionCandidate

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133066/new/

https://reviews.llvm.org/D133066

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133066: fix a typo in comment of AddConversionCandidate

[zhouyizhou via Phabricator via cfe-commits]

zhouyizhou added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133066/new/

https://reviews.llvm.org/D133066

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133066: fix a typo in comment of AddConversionCandidate

[zhouyizhou via Phabricator via cfe-commits]

zhouyizhou added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133066/new/

https://reviews.llvm.org/D133066

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133066: fix a typo in comment of AddConversionCandidate

[zhouyizhou via Phabricator via cfe-commits]

zhouyizhou added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133066/new/

https://reviews.llvm.org/D133066

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133066: fix a typo in comment of AddConversionCandidate

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133066/new/

https://reviews.llvm.org/D133066

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133066: fix a typo in comment of AddConversionCandidate

[zhouyizhou via Phabricator via cfe-commits]

zhouyizhou added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133066/new/

https://reviews.llvm.org/D133066

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133066: fix a typo in comment of AddConversionCandidate

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133066/new/

https://reviews.llvm.org/D133066

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133066: fix a typo in comment of AddConversionCandidate

[zhouyizhou via Phabricator via cfe-commits]

zhouyizhou added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133066/new/

https://reviews.llvm.org/D133066

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133066: fix a typo in comment of AddConversionCandidate

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133066/new/

https://reviews.llvm.org/D133066

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133066: fix a typo in comment of AddConversionCandidate

[zhouyizhou via Phabricator via cfe-commits]

zhouyizhou created this revision.
zhouyizhou added reviewers: MikeStump, riccibruno, rsmith.
Herald added a project: All.
zhouyizhou requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

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 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D133066

Files:
  clang/lib/Sema/SemaOverload.cpp


Index: clang/lib/Sema/SemaOverload.cpp
===
--- clang/lib/Sema/SemaOverload.cpp
+++ clang/lib/Sema/SemaOverload.cpp
@@ -7495,7 +7495,7 @@
 
 // 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() &&


Index: clang/lib/Sema/SemaOverload.cpp
===
--- clang/lib/Sema/SemaOverload.cpp
+++ clang/lib/Sema/SemaOverload.cpp
@@ -7495,7 +7495,7 @@
 
 // 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() &&
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits