[PATCH] D137263: add boundary check for ASTUnresolvedSet::erase

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGb84fd822fa7e: Add boundary check for ASTUnresolvedSet::erase (authored by zhouyizhou, committed by MaskRay). Changed prior to commit: https://revi

[PATCH] D137263: add boundary check for ASTUnresolvedSet::erase

zhouyizhou updated this revision to Diff 473509. zhouyizhou added a comment. Thank Ray for your guidance! I achieved a lot in the process ;-) Thanks Zhouyi CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137263/new/ https://reviews.llvm.org/D137263 Files: clang/include/clang/AST/ASTUn

[PATCH] D137263: add boundary check for ASTUnresolvedSet::erase

MaskRay accepted this revision. MaskRay added a comment. Thanks! Comment at: clang/include/clang/AST/ASTUnresolvedSet.h:73 + void erase(unsigned I) { +if (I == (Decls.size() - 1)) + Decls.pop_back(); `if (I == Decls.size() - 1)` Com

[PATCH] D137263: add boundary check for ASTUnresolvedSet::erase

zhouyizhou updated this revision to Diff 473495. zhouyizhou added a comment. add a test case thank you ;-) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137263/new/ https://reviews.llvm.org/D137263 Files: clang/include/clang/AST/ASTUnresolvedSet.h clang/test/SemaCXX/using-decl-temp

[PATCH] D137263: add boundary check for ASTUnresolvedSet::erase

zhouyizhou added a comment. In D137263#3909722 , @MaskRay wrote: > I find that if I comment out ` > cast(Shadow->getDeclContext())->removeConversion(Shadow);` in > `Sema::HideUsingShadowDecl`, no test fails... So we have a missing coverage > issue. Hi

[PATCH] D137263: add boundary check for ASTUnresolvedSet::erase

zhouyizhou added a comment. 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 > appro

[PATCH] D137263: add boundary check for ASTUnresolvedSet::erase

MaskRay added a comment. We need a regression test. See `clang/test/SemaCXX/using-decl*` for some other using declaration 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/te

[PATCH] D137263: add boundary check for ASTUnresolvedSet::erase

zhouyizhou added a comment. Thank Ray for editing the summary for me ;-) I learned to use fenced code blocks now from your example. Thank you both! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137263/new/ https://reviews.llvm.org/D137263 __

[PATCH] D137263: add boundary check for ASTUnresolvedSet::erase

zhouyizhou added a comment. 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

[PATCH] D137263: add boundary check for ASTUnresolvedSet::erase

MaskRay added a comment. The summary and comments use MarkDown. You comment is currently formatted strangely because you did not use fenced code blocks :) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137263/new/ https://reviews.llvm.org/D137263 ___

[PATCH] D137263: add boundary check for ASTUnresolvedSet::erase

zhouyizhou added a comment. 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 ha

[PATCH] D137263: add boundary check for ASTUnresolvedSet::erase

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. 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. Comment at: clang/include/clang/AST/ASTUn

[PATCH] D137263: add boundary check for ASTUnresolvedSet::erase

zhouyizhou added a comment. Following is the back trace when the assertion fires before my fix: #6 0x77960e96 in __GI___assert_fail (assertion=0x656ed285 "Begin + idx < End", file=0x656ed240 "/tmp/llvm-test/llvm-project.save/clang/include/clang/AST/ASTVector.h", line=112,

[PATCH] D137263: add boundary check for ASTUnresolvedSet::erase

zhouyizhou updated this revision to Diff 472642. zhouyizhou added a comment. We should also consider the situation of erasing the size - 1th element CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137263/new/ https://reviews.llvm.org/D137263 Files: clang/include/clang/AST/ASTUnresolved

[PATCH] D137263: add boundary check for ASTUnresolvedSet::erase

zhouyizhou created this revision. zhouyizhou added reviewers: chandlerc, aprantl, rsmith, rjmccall. Herald added a project: All. zhouyizhou requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. When compile following code with clang (Debug build)