Thank Ray for your guidance!
I achieved a lot in the process ;-)

add a test case
thank you ;-)

In D137263#3909722, @MaskRay wrote:

I find that if I comment out cast<CXXRecordDecl>(Shadow->getDeclContext())->removeConversion(Shadow); in Sema::HideUsingShadowDecl, no test fails... So we have a missing coverage issue.

Hi
I found a counterexample for above case, excited! ! and I could not hesitate to report you that good news ;-)

struct A
{
        struct Nested {};
        operator Nested*() {return 0;};
};

In D137263#3909722, @MaskRay wrote:

Thank Ray for your guidance!

We need a regression test. See clang/test/SemaCXX/using-decl* for some other using declaration tests. Study some tests, find a file which is appropriate, and add a reduced case there.
Use ninja check-clang-semacxx to run clang/test/SemaCXX tests. Use $build/bin/llvm-lit -v clang/test/SemaCXX/using-decl-1.cpp to run one test.

I find that if I comment out cast<CXXRecordDecl>(Shadow->getDeclContext())->removeConversion(Shadow); in Sema::HideUsingShadowDecl, no test fails... So we have a missing coverage issue.

OK, I will try to accomplish all above. There are a lot to learn for me, so please wait for my completion which may take relatively a long time (maybe about 2 weeks).

clang-14 build on Ubuntu 22.04 don't trigger above assertion because clang-14 using g++ -std=c++14 by default:

The instruction and the diassembly is irrelevant to this fix and can be removed.

Done

Thank Ray for editing the summary for me ;-) I learned to use fenced code blocks now from your example.
Thank you both!

In D119493#3403458, @efriedma wrote:

Thank Eli for your comment and guidance and sorry to have carelessly neglected your comment for 7 months!

I'm not sure this is the right fix. If we were handling the pseudo-destructor correctly, I would expect the following to compile successfully:

typedef bool Var;
constexpr bool foobool() {
	return (bool().Var::~Var(), false); 
}

Yes, g++ can compile above code successfully, while we can't.
I am going to try to fix above (I am a beginner who eager to learn ;-)

In D137263#3904184, @MaskRay wrote:

The summary and comments use Markdown. You comment is currently formatted strangely because you did not use fenced code blocks :)
(The commit message does not necessarily use Markdown. Since the summary comes from the commit message, many just use Markdown unconditionally and that is fine.)

In D137263#3903845, @rjmccall wrote:

Good catch! Easy to see how this escaped notice for a decade, but still, it'll be good to fix.

LGTM with a minor improvement.

Thank John for your encouragement and guidance!
I have no write access to LLVM project, could you please commit it for me when you are free?

Following is the back trace when the assertion fires before my fix:
#6 0x00007ffff7960e96 in GI_assert_fail (assertion=0x5555656ed285 "Begin + idx < End",

file=0x5555656ed240 "/tmp/llvm-test/llvm-project.save/clang/include/clang/AST/ASTVector.h", line=112, 
function=0x5555656ed1b0 "T& clang::ASTVector<T>::operator[](unsigned int) [with T = clang::DeclAccessPair; clang::ASTVector<T>::reference = clang::DeclAccessPair&]") at ./assert/assert.c:101

#7 0x000055555f448c05 in clang::ASTVector<clang::DeclAccessPair>::operator[] (this=0x5555691a19b8, idx=0)

at /tmp/llvm-test/llvm-project.save/clang/include/clang/AST/ASTVector.h:112

#8 0x000055555f446e26 in clang::ASTUnresolvedSet::erase (this=0x5555691a19b8, I=0)

at /tmp/llvm-test/llvm-project.save/clang/include/clang/AST/ASTUnresolvedSet.h:72

#9 0x000055555f43e7a3 in clang::CXXRecordDecl::removeConversion (this=0x5555691a18d8, ConvDecl=0x5555691a1c48)

at /tmp/llvm-test/llvm-project.save/clang/lib/AST/DeclCXX.cpp:1797

#10 0x000055555e121349 in clang::Sema::HideUsingShadowDecl (this=0x55556918f990, S=0x5555691acce0, Shadow=0x5555691a1c48)

at /tmp/llvm-test/llvm-project.save/clang/lib/Sema/SemaDeclCXX.cpp:12158

#11 0x000055555e80bc89 in clang::Sema::CheckOverload (this=0x55556918f990, S=0x5555691acce0, New=0x5555691a1da0, Old=...,

Match=@0x7fffffff72b8: 0x0, NewIsUsingDecl=false) at /tmp/llvm-test/llvm-project.save/clang/lib/Sema/SemaOverload.cpp:1063

#12 0x000055555dfdb925 in clang::Sema::CheckFunctionDeclaration (this=0x55556918f990, S=0x5555691acce0, NewFD=0x5555691a1da0,

Previous=..., IsMemberSpecialization=false, DeclIsDefn=true)
at /tmp/llvm-test/llvm-project.save/clang/lib/Sema/SemaDecl.cpp:11356

#13 0x000055555dfd64f3 in clang::Sema::ActOnFunctionDeclarator (this=0x55556918f990, S=0x5555691acce0, D=...,

DC=0x5555691a1918, TInfo=0x5555691a1d68, Previous=..., TemplateParamListsRef=..., AddToScope=@0x7fffffff7b30: true)
at /tmp/llvm-test/llvm-project.save/clang/lib/Sema/SemaDecl.cpp:10205

#14 0x000055555dfc57cb in clang::Sema::HandleDeclarator (this=0x55556918f990, S=0x5555691acce0, D=..., TemplateParamLists=...)

at /tmp/llvm-test/llvm-project.save/clang/lib/Sema/SemaDecl.cpp:6348

--Type <RET> for more, q to quit, c to continue without paging--
#15 0x000055555e0fa7bc in clang::Sema::ActOnCXXMemberDeclarator (this=0x55556918f990, S=0x5555691acce0, AS=clang::AS_public,

D=..., TemplateParameterLists=..., BW=0x0, VS=..., InitStyle=clang::ICIS_NoInit)
at /tmp/llvm-test/llvm-project.save/clang/lib/Sema/SemaDeclCXX.cpp:3477

#16 0x000055555dc5b40a in clang::Parser::ParseCXXInlineMethodDef (this=0x55556919b200, AS=clang::AS_public, AccessAttrs=...,

D=..., TemplateInfo=..., VS=..., PureSpecLoc=...)
at /tmp/llvm-test/llvm-project.save/clang/lib/Parse/ParseCXXInlineMethods.cpp:42

#17 0x000055555dc9b22b in clang::Parser::ParseCXXClassMemberDeclaration (this=0x55556919b200, AS=clang::AS_public,

AccessAttrs=..., TemplateInfo=..., TemplateDiags=0x0)
at /tmp/llvm-test/llvm-project.save/clang/lib/Parse/ParseDeclCXX.cpp:2912

#18 0x000055555dc9d149 in clang::Parser::ParseCXXClassMemberDeclarationWithPragmas (this=0x55556919b200,

AS=@0x7fffffff94f0: clang::AS_public, AccessAttrs=..., TagType=clang::TST_struct, TagDecl=0x5555691a18d8)
at /tmp/llvm-test/llvm-project.save/clang/lib/Parse/ParseDeclCXX.cpp:3343

#19 0x000055555dc9df12 in clang::Parser::ParseCXXMemberSpecification (this=0x55556919b200, RecordLoc=..., AttrFixitLoc=...,

Attrs=..., TagType=25, TagDecl=0x5555691a18d8) at /tmp/llvm-test/llvm-project.save/clang/lib/Parse/ParseDeclCXX.cpp:3547

#20 0x000055555dc97ce3 in clang::Parser::ParseClassSpecifier (this=0x55556919b200, TagTokKind=clang::tok::kw_struct,

StartLoc=..., DS=..., TemplateInfo=..., AS=clang::AS_none, EnteringContext=true, 
DSC=clang::Parser::DeclSpecContext::DSC_top_level, Attributes=...)
at /tmp/llvm-test/llvm-project.save/clang/lib/Parse/ParseDeclCXX.cpp:2063

#21 0x000055555dc7504a in clang::Parser::ParseDeclarationSpecifiers (this=0x55556919b200, DS=..., TemplateInfo=...,

AS=clang::AS_none, DSContext=clang::Parser::DeclSpecContext::DSC_top_level, LateAttrs=0x0)
at /tmp/llvm-test/llvm-project.save/clang/lib/Parse/ParseDecl.cpp:4144

#22 0x000055555dc4a70e in clang::Parser::ParseDeclOrFunctionDefInternal (this=0x55556919b200, Attrs=..., DS=...,

AS=clang::AS_none) at /tmp/llvm-test/llvm-project.save/clang/lib/Parse/Parser.cpp:1087

#23 0x000055555dc4adb1 in clang::Parser::ParseDeclarationOrFunctionDefinition (this=0x55556919b200, Attrs=..., DS=0x0,

AS=clang::AS_none) at /tmp/llvm-test/llvm-project.save/clang/lib/Parse/Parser.cpp:1193

#24 0x000055555dc4a260 in clang::Parser::ParseExternalDeclaration (this=0x55556919b200, Attrs=..., DS=0x0)

at /tmp/llvm-test/llvm-project.save/clang/lib/Parse/Parser.cpp:1019

#25 0x000055555dc48fac in clang::Parser::ParseTopLevelDecl (this=0x55556919b200, Result=...,

ImportState=@0x7fffffffa97c: clang::Sema::ModuleImportState::NotACXX20Module)
at /tmp/llvm-test/llvm-project.save/clang/lib/Parse/Parser.cpp:737

#26 0x000055555dc43f29 in clang::ParseAST (S=..., PrintStats=false, SkipFunctionBodies=false)
--Type <RET> for more, q to quit, c to continue without paging--

at /tmp/llvm-test/llvm-project.save/clang/lib/Parse/ParseAST.cpp:162

#27 0x000055555b4c6aaf in clang::ASTFrontendAction::ExecuteAction (this=0x55556911d8e0)

at /tmp/llvm-test/llvm-project.save/clang/lib/Frontend/FrontendAction.cpp:1152

#28 0x000055555bebe736 in clang::CodeGenAction::ExecuteAction (this=0x55556911d8e0)

at /tmp/llvm-test/llvm-project.save/clang/lib/CodeGen/CodeGenAction.cpp:1144

#29 0x000055555b4c635e in clang::FrontendAction::Execute (this=0x55556911d8e0)

at /tmp/llvm-test/llvm-project.save/clang/lib/Frontend/FrontendAction.cpp:1045

#30 0x000055555b3f45cd in clang::CompilerInstance::ExecuteAction (this=0x555569118ba0, Act=...)

at /tmp/llvm-test/llvm-project.save/clang/lib/Frontend/CompilerInstance.cpp:1039

#31 0x000055555b667bce in clang::ExecuteCompilerInvocation (Clang=0x555569118ba0)

at /tmp/llvm-test/llvm-project.save/clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:266

#32 0x0000555556c05a6d in cc1_main (Argv=..., Argv0=0x7fffffffdbc0 "/tmp/llvm-test/llvm-project.save/build/bin/clang-16",

MainAddr=0x555556bf616a <GetExecutablePath[abi:cxx11](char const*, bool)>)
at /tmp/llvm-test/llvm-project.save/clang/tools/driver/cc1_main.cpp:250

#33 0x0000555556bf79ef in ExecuteCC1Tool (ArgV=...) at /tmp/llvm-test/llvm-project.save/clang/tools/driver/driver.cpp:322
#34 0x0000555556bf81a0 in clang_main (Argc=64, Argv=0x7fffffffd628)

at /tmp/llvm-test/llvm-project.save/clang/tools/driver/driver.cpp:397

#35 0x0000555556c252a1 in main (argc=64, argv=0x7fffffffd628)

at /tmp/llvm-test/llvm-project.save/build/tools/clang/tools/driver/clang-driver.cpp:11

We should also consider the situation of erasing the size - 1th element

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

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.

my current progress - succesfully build commit cba72b1f620fd on debian6:

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

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.

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.

I have no write access to LLVM, could you commit for me at your convenience ;-)

In D129953#3657920, @njames93 wrote:

While you're here could you also put full stops( or periods 🙃) at the end of the sentences.

In D121498#3385896, @erichkeane wrote:

LGTM! Let me know if you need me to commit this for you (and if so, what name/email address you'd like it committed under).

Thank you for your guidance!
I do need you to commit this for me. I want my name and following email address be committed with the patch:
Zhouyi Zhou<zhouzhouyi@gmail.com>

In D121498#3379222, @erichkeane wrote:

This seems reasonable to me, but can you please add a lit test and fix the format of the code in your commit message?

In D115014#3169444, @simon_tatham wrote:

Ah, now I see what you mean – I didn't look far enough!

I don't know this code well (in fact I'm not sure why you picked me as a reviewer), but off the top of my head: the question is not just whether tokens_iterator happens to be the same thing as const Token * now. It's whether it's guaranteed to stay that way in the future.

One of the purposes of these iterator types is that they form an opaque abstraction layer: the client uses the type in only the ways that are guaranteed to work by the iterator specification, and then the implementation can completely change without breaking any client.

If you make a change like this, and later SmallVector::iterator changes to some other legal implementation, or tokens_iterator changes to be something other than a SmallVector::iterator, then this change will have to be reverted.

In D115014#3169400, @simon_tatham wrote:

I don't think so, I'm afraid. If you look at the definition of MacroInfo::tokens_begin() in clang/include/clang/Lex/MacroInfo.h, you see that it doesn't return a raw Token *: it returns a C++ iterator object.

using tokens_iterator = SmallVectorImpl<Token>::const_iterator;

tokens_iterator tokens_begin() const { return ReplacementTokens.begin(); }

So you do have to dereference the iterator to get a Token &, and then address-take that to turn it into a Token *.

ping
Just a ping, to see if there are any comments :-P

I create this patch because I am also a Linux hobbyist, in Linux kernel, return statement in a void function is always standalone. For example in kernel/event/core.c:

In D109408#3018830, @ldionne wrote:

In D109408#2996128, @zhouyizhou wrote:

In D109408#2996092, @Quuxplusone wrote:

I was going to land this just now, but then realized that I've lost the thread of why we're doing most of this. Why are we touching clang/test/SemaCXX/? So I'm going to land just the fixing of the curly braces in libcxxabi/src/.

Hi,
Very sorry to bring you so much trouble.
I am new to LLVM, and I am only intend to fix the curly braces in libcxxabi/src/,
I followed https://www.llvm.org/docs/Phabricator.html#phabricator-reviews to make a patch using:
git diff -U999999 u > /tmp/mypatch.patch
I am also very confused why clang/test/SemaCXX/ appears in mypatch.patch.
Sorry again.
And thanks for landing the fixing of the curly braces in libcxxabi/src/ for me.

Zhouyi

When you submit patches, please follow https://libcxx.llvm.org/Contributing.html. In particular, you should follow these instructions for submitting using arc diff -- it works a lot better than the web interface: https://www.llvm.org/docs/Phabricator.html#requesting-a-review-via-the-command-line.

Thanks for the patch!

In D109408#2996092, @Quuxplusone wrote:

I was going to land this just now, but then realized that I've lost the thread of why we're doing most of this. Why are we touching clang/test/SemaCXX/? So I'm going to land just the fixing of the curly braces in libcxxabi/src/.

Hi,
I have no write access to LLVM, can you commit D109408 for me?
Thanks a lot
Zhouyi Zhou <zhouzhouyi@gmail.com>

In D109408#2988417, @ldionne wrote:

FWIW, I dislike that we don't indent stuff at all inside namespaces -- often I find it useful to indent things inside short-lived namespaces. But let's go for simplicity and consistency.

Did you look into clang-formatting other files in libc++abi? What would the diff look like?

In D109408#2988417, @ldionne wrote:

FWIW, I dislike that we don't indent stuff at all inside namespaces -- often I find it useful to indent things inside short-lived namespaces. But let's go for simplicity and consistency.

Did you look into clang-formatting other files in libc++abi? What would the diff look like?

Thanks for reviewing
I don't have the write access to LLVM, could you please commit this patch for me?
Signed-off-by: Zhouyi Zhou <zhouzhouyi@gmail.com>

In D104604#2849676, @xgupta wrote:

It seems you need someone to commit this patch? If so you need to share your name and email address. So that someone with commit access can commit it for you.

use clang-format to fix the function TransformNestedNameSpecifierLoc

I do some "grep" in clang/lib directory:
llvm-project/clang/lib#find . -name "*.*"|xargs grep -n -B 2 "break;"|grep -A 2 "}"

I use git diff -U99999 instead