simark added a comment. In https://reviews.llvm.org/D35894#1008861, @ilya-biryukov wrote:
> Only the naming of fields left. > > Also, please make sure to run `git-clang-format` on the code before > submitting to make sure it's formatted properly. Ahh, I was running clang-format by hand, didn't know about git-clang-format (and obviously forgot some files). Thanks. ================ Comment at: clangd/ClangdLSPServer.cpp:331 + if (!H) { + replyError(ErrorCode::InternalError, + llvm::toString(H.takeError())); ---------------- ilya-biryukov wrote: > NIT: use `return replyError` to be consistent with other methods. Ok. ================ Comment at: clangd/XRefs.cpp:354 + + return Name; +} ---------------- ilya-biryukov wrote: > simark wrote: > > ilya-biryukov wrote: > > > We should call `flush()` before returning `Name` here. The > > > `raw_string_ostream` is buffered. > > I replaced it with `Stream.str()`. > Also works, albeit less efficient (will probably do a copy where move is > enough). Ah, didn't think about that. I'll change it to flush + return Name. ================ Comment at: unittests/clangd/XRefsTests.cpp:262 + struct OneTest { + StringRef input; + StringRef expectedHover; ---------------- ilya-biryukov wrote: > simark wrote: > > ilya-biryukov wrote: > > > NIT: LLVM uses `UpperCamelCase` for field names. > > Ok, I fixed all the structs this patch adds. > Argh, sorry that I didn't mention it before, but we have an exception for > structs in `Procotol.h`, they follow the spelling in the LSP spec (i.e. > should be `lowerCamelCase`). > Haha, no problem, changing them back is one keystroke away. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D35894 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits