[PATCH] D89852: [clangd] Get rid of llvm::Optional in Remote- and LocalIndexRoot; NFC

[Aleksandr Platonov via Phabricator via cfe-commits]

ArcsinX added inline comments.


================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:60
+    llvm::StringRef Path(this->RemoteIndexRoot);
+    if (!is_separator(this->RemoteIndexRoot.back(),
+                      llvm::sys::path::Style::posix))
----------------
kbobyrev wrote:
> kadircet wrote:
> > ArcsinX wrote:
> > > I know this already commited, but maybe we could use `llvm::sys::path::is_separator` instead of `is_separator` to be consistent with other function calls from `llvm::sys::path` namespace in this file.
> > > 
> > > P.S. `llvm::sys::path::is_separator` checks a single char, but `llvm::sys::path::get_separator` returns string. Can not understand why. Could there be a multi-char separator in the future?
> > Because `llvm::sys::path::get_separator` returns a null terminated string.
> > 
> > > Could there be a multi-char separator in the future?
> > 
> > I would expect the person doing such a change to also update `is_separator`.
> I think it'd be better to strip `llvm::sys::path` elsewhere and simply rely on ADL. Spelling `llvm::sys::path::` twice does not really add much value and simply takes more space. I'll send a separate patch.
> Because llvm::sys::path::get_separator returns a null terminated string.
Why does not `llvm::sys::path::get_separator()` return a single char instead of a null-terminated string?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89852/new/

https://reviews.llvm.org/D89852