[clang-tools-extra] r322509 - [clangd] Improve const-correctness of Symbol->Detail. NFC

[Sam McCall via cfe-commits]

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