nridge added inline comments.
================ Comment at: clang-tools-extra/clangd/CodeComplete.cpp:2133 if (const auto *EnumDecl = dyn_cast<clang::EnumDecl>(ND.getDeclContext())) - return InTopLevelScope(*EnumDecl) && !EnumDecl->isScoped(); ---------------- tom-anders wrote: > nridge wrote: > > Just to make sure I understand: > > > > By also removing the `!isScoped()` condition, in addition to changing the > > behaviour for the scenario described in > > https://github.com/clangd/clangd/issues/1082 (enum declared at class > > scope), you are also changing the behaviour for scenarios like this: > > > > ``` > > enum class Foo { Bar }; // at any scope, including global > > ``` > > > > Completing `Bar` will now offer `Foo::Bar` when previously it didn't. > > > > Is this your intention? > Ah sorry, that is indeed not what I described in the issue - but yeah this > change is intended here, IMO it's more consistent from a user perspective: > Why should **all-scopes**-completion ignore **scoped** enums? I have no objection to this additional change personally, but to play devil's advocate a bit: one could argue that for scoped enums, because the enumeration name needs to be mentioned at every use, the enumerator names are more likely to be shorter/common words, such that they may appear in multiple enumerations across a project, making the completion results more noisy. Perhaps, it would make sense to split this out into a separate patch, so that in case it turns out to be controversial, it can be reverted without affecting the other part of the fix? ================ Comment at: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp:1334 + AllOf(qName("Color3"), forCodeCompletion(true)), + AllOf(qName("Color3::Blue"), forCodeCompletion(true)), AllOf(qName("ns"), forCodeCompletion(true)), ---------------- I think it would be nice to add a test case to `CodeCompleteTests.cpp` as well. (For example, it was not obvious to me that in the scoped enum case, completing the enumerator name would correctly insert the enumeration name as part of the scope, until I actually tried it in the editor.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136925/new/ https://reviews.llvm.org/D136925 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
