sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
LG from my side.
If you have unit tests in the next couple of days, happy to take a look,
otherwise go ahead once Alex/Ben are happy.
(I'm at work today/tomorrow and then I'm going to be away through new year's,
so don't wait for me)
================
Comment at: clangd/ClangdServer.cpp:529
+
+ WorkScheduler.runWithAST("CursorInfo", File, Bind(Action, std::move(CB)));
+}
----------------
sammccall wrote:
> nit: SymbolInfo
(This still says CursorInfo in the runWithAST call)
================
Comment at: clangd/ClangdServer.h:204
+ /// Get symbol info for given position.
+ void symbolInfo(PathRef File, Position Pos,
----------------
Maybe mention "clangd extension" here.
We should really set up some more structured docs for these one day, but a
comment is something.
================
Comment at: clangd/Protocol.cpp:429
+ return json::Object{
+ {"name", P.name},
+ {"containerName", P.containerName},
----------------
sammccall wrote:
> for each of the attributes that can be logically absent, we should probably
> serialize it conditionally (or serialize it as null).
>
> We're usually happy enough to use sentinel values like "" to mean missing
> internally, but we try not to send them over the wire.
> (SymbolInformation isn't a great example unfortunately...)
elsewhere we tend to write:
```
json::Object Result{
... mandatory fields ...
};
```
if (!P.name.empty())
Result["name"] = P.name;
```
etc. I find this a little bit more readable than the ternary expressions (due
to the need for conversions) but up to you.
================
Comment at: clangd/XRefs.cpp:785
+ }
+ Results.emplace_back(std::move(NewSymbol));
+ }
----------------
jkorous wrote:
> sammccall wrote:
> > nit: push_back (and below)
> Sure, no problem. I have to admit I thought these two are the same but I
> guess you pointed this out because there's a functional difference. I'd
> appreciate if you could advise where can I learn the details.
Here the behavior is identical, so this is really a style thing.
`emplace_back` is a more powerful function, with semantics "make a new
element": it will forward *any* argument list to a constructor of Symbol,
selected by overload resolution.
`push_back` is a more constrained function, with semantics "insert this
element": it takes one Symbol and forwards it to the move constructor (or copy
constructor, as appropriate).
They're equivalent here because `emplace_back` can select the move-constructor
when passed a single `Symbol&&`. But it communicates intent less clearly, so
humans have a harder time (harder to understand the code) as does the compiler
(error messages are worse).
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D54799/new/
https://reviews.llvm.org/D54799
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
