[PATCH] D41730: [clangd] Use ToolExecutor to write the global-symbol-builder tool.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

The implementation looks good, but to see whether sam has high-level comments on this.



================
Comment at: clangd/index/SymbolYAML.cpp:140
   llvm::yaml::Output Yout(OS);
   for (Symbol S : Symbols) // copy: Yout<< requires mutability.
     Yout<< S;
----------------
The function could be simplified by using the SymbolToYAML below.

```
std::string Str;
for (const Symbol& S : Symbols) {
   Str += SymbolToYAML(S);
}
```


================
Comment at: clangd/index/SymbolYAML.cpp:145
 
+std::string SymbolToYAML(const Symbol& Sym) {
+  std::string Str;
----------------
might be use `Symbol` as parameter type.


================
Comment at: clangd/index/SymbolYAML.h:30
 
+// Convert a single symbol to YAML-format string.
+std::string SymbolToYAML(const Symbol &Sym);
----------------
might be also mention `The YAML result is safe to concatenate`.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41730