[PATCH] D51598: [clangd] Avoid crashes in override completions
This revision was automatically updated to reflect the committed changes. Closed by commit rL341322: [clangd] Avoid crashes in override completions (authored by ibiryukov, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM https://reviews.llvm.org/D51598 Files: clang-tools-extra/trunk/clangd/CodeComplete.cpp clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp Index: clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp === --- clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp +++ clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp @@ -1765,6 +1765,21 @@ Not(Contains(Labeled("void vfunc(bool param) override"); } +TEST(CompletionTest, OverridesNonIdentName) { + // Check the completions call does not crash. + completions(R"cpp( +struct Base { + virtual ~Base() = 0; + virtual operator int() = 0; + virtual Base& operator+(Base&) = 0; +}; + +struct Derived : Base { + ^ +}; + )cpp"); +} + TEST(SpeculateCompletionFilter, Filters) { Annotations F(R"cpp($bof^ $bol^ Index: clang-tools-extra/trunk/clangd/CodeComplete.cpp === --- clang-tools-extra/trunk/clangd/CodeComplete.cpp +++ clang-tools-extra/trunk/clangd/CodeComplete.cpp @@ -210,7 +210,7 @@ // These are stored by name to make querying fast in the later step. llvm::StringMap> Overrides; for (auto *Method : CR->methods()) { -if (!Method->isVirtual()) +if (!Method->isVirtual() || !Method->getIdentifier()) continue; Overrides[Method->getName()].push_back(Method); } @@ -221,14 +221,14 @@ if (!BR) continue; for (auto *Method : BR->methods()) { - if (!Method->isVirtual()) + if (!Method->isVirtual() || !Method->getIdentifier()) continue; const auto it = Overrides.find(Method->getName()); bool IsOverriden = false; if (it != Overrides.end()) { for (auto *MD : it->second) { // If the method in current body is not an overload of this virtual - // function, that it overrides this one. + // function, then it overrides this one. if (!S->IsOverload(MD, Method, false)) { IsOverriden = true; break; Index: clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp === --- clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp +++ clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp @@ -1765,6 +1765,21 @@ Not(Contains(Labeled("void vfunc(bool param) override"); } +TEST(CompletionTest, OverridesNonIdentName) { + // Check the completions call does not crash. + completions(R"cpp( +struct Base { + virtual ~Base() = 0; + virtual operator int() = 0; + virtual Base& operator+(Base&) = 0; +}; + +struct Derived : Base { + ^ +}; + )cpp"); +} + TEST(SpeculateCompletionFilter, Filters) { Annotations F(R"cpp($bof^ $bol^ Index: clang-tools-extra/trunk/clangd/CodeComplete.cpp === --- clang-tools-extra/trunk/clangd/CodeComplete.cpp +++ clang-tools-extra/trunk/clangd/CodeComplete.cpp @@ -210,7 +210,7 @@ // These are stored by name to make querying fast in the later step. llvm::StringMap> Overrides; for (auto *Method : CR->methods()) { -if (!Method->isVirtual()) +if (!Method->isVirtual() || !Method->getIdentifier()) continue; Overrides[Method->getName()].push_back(Method); } @@ -221,14 +221,14 @@ if (!BR) continue; for (auto *Method : BR->methods()) { - if (!Method->isVirtual()) + if (!Method->isVirtual() || !Method->getIdentifier()) continue; const auto it = Overrides.find(Method->getName()); bool IsOverriden = false; if (it != Overrides.end()) { for (auto *MD : it->second) { // If the method in current body is not an overload of this virtual - // function, that it overrides this one. + // function, then it overrides this one. if (!S->IsOverload(MD, Method, false)) { IsOverriden = true; break; ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51598: [clangd] Avoid crashes in override completions
ilya-biryukov added inline comments. Comment at: unittests/clangd/CodeCompleteTests.cpp:1770 + // Check the completions call does not crash. + completions(R"cpp( +struct Base { ioeric wrote: > ilya-biryukov wrote: > > Was wondering if testing for crashes LG this way, or adding more assertions > > might make sense too > Hmm, I think this is okay, but I'd probably do some sanity check on the > results, just to make it look less odd ;) Exactly my feelings: this looks odd. However, couldn't come up with a decent sanity check at this point. The reason is: we don't store enough information to tell override completion from non-override ones. It means I can assert something like `Not(Contains(Labelled("~Base() override")))`, but lots of broken outputs can still make the test pass, e.g.: - `void ~Base() override` - `~Derived() override` - ... Will probably keep as this and think how to factor out overriden completions from the results better... Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51598 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51598: [clangd] Avoid crashes in override completions
ioeric added inline comments. Comment at: unittests/clangd/CodeCompleteTests.cpp:1770 + // Check the completions call does not crash. + completions(R"cpp( +struct Base { ilya-biryukov wrote: > Was wondering if testing for crashes LG this way, or adding more assertions > might make sense too Hmm, I think this is okay, but I'd probably do some sanity check on the results, just to make it look less odd ;) Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51598 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51598: [clangd] Avoid crashes in override completions
ilya-biryukov added inline comments. Comment at: unittests/clangd/CodeCompleteTests.cpp:1770 + // Check the completions call does not crash. + completions(R"cpp( +struct Base { Was wondering if testing for crashes LG this way, or adding more assertions might make sense too Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51598 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51598: [clangd] Avoid crashes in override completions
ilya-biryukov created this revision. ilya-biryukov added reviewers: kadircet, ioeric, hokein, sammccall. Herald added subscribers: arphaman, jkorous, MaskRay. NamedDecl::getName cannot be called on non-identifier names. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51598 Files: clangd/CodeComplete.cpp unittests/clangd/CodeCompleteTests.cpp Index: unittests/clangd/CodeCompleteTests.cpp === --- unittests/clangd/CodeCompleteTests.cpp +++ unittests/clangd/CodeCompleteTests.cpp @@ -1765,6 +1765,21 @@ Not(Contains(Labeled("void vfunc(bool param) override"); } +TEST(CompletionTest, OverridesNonIdentName) { + // Check the completions call does not crash. + completions(R"cpp( +struct Base { + virtual ~Base() = 0; + virtual operator int() = 0; + virtual Base& operator+(Base&) = 0; +}; + +struct Derived : Base { + ^ +}; + )cpp"); +} + TEST(SpeculateCompletionFilter, Filters) { Annotations F(R"cpp($bof^ $bol^ Index: clangd/CodeComplete.cpp === --- clangd/CodeComplete.cpp +++ clangd/CodeComplete.cpp @@ -210,7 +210,7 @@ // These are stored by name to make querying fast in the later step. llvm::StringMap> Overrides; for (auto *Method : CR->methods()) { -if (!Method->isVirtual()) +if (!Method->isVirtual() || !Method->getIdentifier()) continue; Overrides[Method->getName()].push_back(Method); } @@ -221,14 +221,14 @@ if (!BR) continue; for (auto *Method : BR->methods()) { - if (!Method->isVirtual()) + if (!Method->isVirtual() || !Method->getIdentifier()) continue; const auto it = Overrides.find(Method->getName()); bool IsOverriden = false; if (it != Overrides.end()) { for (auto *MD : it->second) { // If the method in current body is not an overload of this virtual - // function, that it overrides this one. + // function, then it overrides this one. if (!S->IsOverload(MD, Method, false)) { IsOverriden = true; break; Index: unittests/clangd/CodeCompleteTests.cpp === --- unittests/clangd/CodeCompleteTests.cpp +++ unittests/clangd/CodeCompleteTests.cpp @@ -1765,6 +1765,21 @@ Not(Contains(Labeled("void vfunc(bool param) override"); } +TEST(CompletionTest, OverridesNonIdentName) { + // Check the completions call does not crash. + completions(R"cpp( +struct Base { + virtual ~Base() = 0; + virtual operator int() = 0; + virtual Base& operator+(Base&) = 0; +}; + +struct Derived : Base { + ^ +}; + )cpp"); +} + TEST(SpeculateCompletionFilter, Filters) { Annotations F(R"cpp($bof^ $bol^ Index: clangd/CodeComplete.cpp === --- clangd/CodeComplete.cpp +++ clangd/CodeComplete.cpp @@ -210,7 +210,7 @@ // These are stored by name to make querying fast in the later step. llvm::StringMap> Overrides; for (auto *Method : CR->methods()) { -if (!Method->isVirtual()) +if (!Method->isVirtual() || !Method->getIdentifier()) continue; Overrides[Method->getName()].push_back(Method); } @@ -221,14 +221,14 @@ if (!BR) continue; for (auto *Method : BR->methods()) { - if (!Method->isVirtual()) + if (!Method->isVirtual() || !Method->getIdentifier()) continue; const auto it = Overrides.find(Method->getName()); bool IsOverriden = false; if (it != Overrides.end()) { for (auto *MD : it->second) { // If the method in current body is not an overload of this virtual - // function, that it overrides this one. + // function, then it overrides this one. if (!S->IsOverload(MD, Method, false)) { IsOverriden = true; break; ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits