sammccall added inline comments.
================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:96
+ } Counter(Request->DebugString());
+ return std::forward<IndexCall>(Call)(
+ *Index, ClangdRequest, [&](const StreamType &Item) {
----------------
kbobyrev wrote:
> sammccall wrote:
> > I don't think you need forward here... just take the param by const
> > reference?
> How do I do that? The problem is that some functions return `bool` and one
> `void`, so I can't really assign the result to variable and then return it.
Currently you have
```
template <typename IndexCall>(IndexCall&& Call) {
forward<IndexCall>(Call);
}
```
but I think this would work fine here:
```
template <typename IndexCall>(const IndexCall& Call) {
IndexCall(Call);
}
```
(nothing particularly wrong with this use of forward, but "more template goop"
feels particularly expensive here.
================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:49
+ llvm::cl::desc("Pretty-print JSON output"),
+ llvm::cl::init(true),
+};
----------------
kbobyrev wrote:
> sammccall wrote:
> > kadircet wrote:
> > > this sounds like a debug feature, do we really want it to be true by
> > > default?
> > This option is not worth having at all, IMO.
> Why not? It's relatively hard to read trace files otherwise. Do you think
> they should be pretty-printed by default?
FWIW, I've never had to read them except when initially developing/debugging
the trace feature.
(The most I have to do is fix up a broken end-of-file, and events are
one-per-line in non-pretty anyway...)
I think either always-on or always-off is better than having a flag.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D84499/new/
https://reviews.llvm.org/D84499
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
