https://github.com/llvm-beanz approved this pull request.

Sorry for the extreme delay in review. I think everyone trying to keep up with 
clangd reviews has been swamped and pretty over subscribed.

The implementation here looks clean and follows existing conventions in clangd. 
It is also quite well commented. The test coverage seems solid.

The one concern I have is that there could be quite a few string allocations 
and reallocations. Generally I prefer to see code building up strings using 
`SmallString` and `raw_svector_ostream` to build up the string.

That said, much of the code in other clangd tweaks uses std::string pretty 
aggressively so I think this is probably fine and we can revisit for 
performance if we encounter issues.

https://github.com/llvm/llvm-project/pull/139348
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to