Author: sammccall Date: Mon Jan 15 12:09:09 2018 New Revision: 322509 URL: http://llvm.org/viewvc/llvm-project?rev=322509&view=rev Log: [clangd] Improve const-correctness of Symbol->Detail. NFC
Summary: This would have caught a bug I wrote in an early version of D42049, where an index user could overwrite data internal to the index because the Symbol is not deep-const. The YAML traits are now a bit more verbose, but separate concerns a bit more nicely: ArenaPtr can be reused for other similarly-allocated objects, including scalars etc. Reviewers: hokein Subscribers: klimek, ilya-biryukov, cfe-commits, ioeric Differential Revision: https://reviews.llvm.org/D42059 Modified: clang-tools-extra/trunk/clangd/index/Index.cpp clang-tools-extra/trunk/clangd/index/Index.h clang-tools-extra/trunk/clangd/index/SymbolYAML.cpp Modified: clang-tools-extra/trunk/clangd/index/Index.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/Index.cpp?rev=322509&r1=322508&r2=322509&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/index/Index.cpp (original) +++ clang-tools-extra/trunk/clangd/index/Index.cpp Mon Jan 15 12:09:09 2018 @@ -64,13 +64,12 @@ static void own(Symbol &S, DenseSet<Stri if (S.Detail) { // Copy values of StringRefs into arena. auto *Detail = Arena.Allocate<Symbol::Details>(); - Detail->Documentation = S.Detail->Documentation; - Detail->CompletionDetail = S.Detail->CompletionDetail; - S.Detail = Detail; - + *Detail = *S.Detail; // Intern the actual strings. - Intern(S.Detail->Documentation); - Intern(S.Detail->CompletionDetail); + Intern(Detail->Documentation); + Intern(Detail->CompletionDetail); + // Replace the detail pointer with our copy. + S.Detail = Detail; } } Modified: clang-tools-extra/trunk/clangd/index/Index.h URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/Index.h?rev=322509&r1=322508&r2=322509&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/index/Index.h (original) +++ clang-tools-extra/trunk/clangd/index/Index.h Mon Jan 15 12:09:09 2018 @@ -156,7 +156,7 @@ struct Symbol { }; // Optional details of the symbol. - Details *Detail = nullptr; // FIXME: should be const + const Details *Detail = nullptr; // FIXME: add definition location of the symbol. // FIXME: add all occurrences support. Modified: clang-tools-extra/trunk/clangd/index/SymbolYAML.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/SymbolYAML.cpp?rev=322509&r1=322508&r2=322509&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/index/SymbolYAML.cpp (original) +++ clang-tools-extra/trunk/clangd/index/SymbolYAML.cpp Mon Jan 15 12:09:09 2018 @@ -60,27 +60,39 @@ template <> struct MappingTraits<SymbolI } }; -template <> -struct MappingTraits<Symbol::Details *> { - static void mapping(IO &io, Symbol::Details *&Detail) { - if (!io.outputting()) { - assert(io.getContext() && "Expecting an arena (as context) to allocate " - "data for new symbols."); - Detail = static_cast<llvm::BumpPtrAllocator *>(io.getContext()) - ->Allocate<Symbol::Details>(); - } else if (!Detail) { - // Detail is optional in outputting. - return; - } - assert(Detail); - io.mapOptional("Documentation", Detail->Documentation); - io.mapOptional("CompletionDetail", Detail->CompletionDetail); +template <> struct MappingTraits<Symbol::Details> { + static void mapping(IO &io, Symbol::Details &Detail) { + io.mapOptional("Documentation", Detail.Documentation); + io.mapOptional("CompletionDetail", Detail.CompletionDetail); } }; +// A YamlIO normalizer for fields of type "const T*" allocated on an arena. +// Normalizes to Optional<T>, so traits should be provided for T. +template <typename T> struct ArenaPtr { + ArenaPtr(IO &) {} + ArenaPtr(IO &, const T *D) { + if (D) + Opt = *D; + } + + const T *denormalize(IO &IO) { + assert(IO.getContext() && "Expecting an arena (as context) to allocate " + "data for read symbols."); + if (!Opt) + return nullptr; + return new (*static_cast<llvm::BumpPtrAllocator *>(IO.getContext())) + T(std::move(*Opt)); // Allocate a copy of Opt on the arena. + } + + llvm::Optional<T> Opt; +}; + template <> struct MappingTraits<Symbol> { static void mapping(IO &IO, Symbol &Sym) { MappingNormalization<NormalizedSymbolID, SymbolID> NSymbolID(IO, Sym.ID); + MappingNormalization<ArenaPtr<Symbol::Details>, const Symbol::Details *> + NDetail(IO, Sym.Detail); IO.mapRequired("ID", NSymbolID->HexString); IO.mapRequired("Name", Sym.Name); IO.mapRequired("Scope", Sym.Scope); @@ -92,8 +104,7 @@ template <> struct MappingTraits<Symbol> IO.mapOptional("CompletionSnippetInsertText", Sym.CompletionSnippetInsertText); - if (!IO.outputting() || Sym.Detail) - IO.mapOptional("Detail", Sym.Detail); + IO.mapOptional("Detail", NDetail->Opt); } }; _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
