sammccall marked 3 inline comments as done.
sammccall added a comment.
Thanks!
================
Comment at: clangd/Context.h:77
+ /// See WithContext and WithContextValue for how to do this safely.
+ static Context ¤t();
----------------
ilya-biryukov wrote:
> Maybe we could return `const Context&` here and have a separate
> `setCurrent()` method for replacing the current context?
> Most of the clients shouldn't mutate the contexts anyway, and finding the few
> that do seems easier if there's a separate method for that.
Done (named `swap`).
In fact the only caller is `WithContext`, which is a good sign.
It would be possible to make `swap()` private, since WithContext is in this
file. I tend to lean against it because embedders will do explicit context
manipulation and WithContext may not be the right primitive for them. So I've
left the WithContext guidance as "just a comment". Could be convinced though.
================
Comment at: clangd/Context.h:201
+ WithContext &operator=(const WithContext &) = delete;
+ WithContext(WithContext &&) = default; // Allow return-by-value.
+ WithContext &operator=(WithContext &&) = delete;
----------------
ilya-biryukov wrote:
> Where do we use it? Can't we make move ctor deleted too?
Good catch! This was used in an old version of the patch.
================
Comment at: clangd/Trace.cpp:131
+ return Context::current().clone();
+ WithContextValue WithArgs{std::unique_ptr<json::obj>(Args)};
+ return T->beginSpan(Name, Args);
----------------
ilya-biryukov wrote:
> Maybe use parentheses instead of braces for initialization?
Braces here are to avoid most-vexing-parse: with parens this is a function
declaration.
(I'm not sure this is worth a comment, as the compiler flags it if you change
it)
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42517
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
