- User Since
- Aug 26 2016, 6:53 AM (103 w, 1 d)
Sun, Aug 12
I also prefer this version over suppressing the warning with the pragma.
Wed, Aug 8
Nevermind, this is like 5x slower to load the chromium CDB. probably because of all the little non-arena allocations.
I'll profile and fix it when I get bored again.
(just updating description)
Tue, Aug 7
Couple of thoughts. (Technically I'm out on leave so will let Jan/Ilya
review implementation and happy with whatever you decide)
Thu, Aug 2
Capitalizing sounds nice. +1 to just do it without an option.
Sun, Jul 29
Makes sense, thanks!
Sat, Jul 28
Usage of JSON.h looks pretty good, thanks for doing this migration!
Tue, Jul 24
Just a couple of high-level comments here:
- I'm not sure we can/should commit to supporting editor-based file watching forever.
- One natural long-term direction would be to get this functionality into JSONCompilationDatabase, and clients of that don't have an LSP client to provide events
- client support is going to remain uneven in the foreseeable future. LSP is as always underspecified here, and there's no chance that every editor plugin is going to do a reasonable job of recursive file watching, even among those that advertise support for it.
- if/when we have a native implementation, supporting multiple mechanisms with different layering requirements to get at most a 2x win in watcher resource usage seems like a dubious way to spend our complexity budget.
- it looks like the current design throws away all compilation databases (via the global DB) when *any* compile_commands.json changes. I think it's important that other stateful compilation databases aren't thrown away for unrelated reasons. e.g. the Bazel DB takes nontrivial time to reinitialize.
Thanks for the polish!
Last few nits, but feel free to land once you're happy.
This looks really nice.
Iterator implementations can be simplified a bit I think.
Remove one last -style flag.
Thanks, good catch!
Mon, Jul 23
No opinion on the boost style thing.
Looks really nice! Only major issue is the query trigrams don't look right.
Otherwise, some style nits and fixes that seem to have gotten lost.
Or make operator bool explicit
A few more comments about the bits I understand, but waiting mostly on the documentation.
Fri, Jul 20
Sorry for the delay here, and I'm sorry I haven't been into the patches in detail again yet - crazy week. After experiments, I am convinced the Transport abstraction patch can turn into something nice if we want XPC vs JSON to be a pure transport-level difference with the same semantics. But you need to decide the approach here based on your requirements.
We're hitting assertions after this revision when running TensorFlow tests
(specifically this test on GPU: https://github.com/tensorflow/tensorflow/blob/master/tensorflow/compiler/tests/gather_test.py)
Jul 19 2018
Thanks for sending this early!
Rough interface comments - mostly looks good though!
(just .h files. +1 to eric's comments except where noted)
Jul 17 2018
Jul 16 2018
Just an initial couple of thoughts here, haven't yet been through in detail.
Sorry it took so long for these to get attention.
Starting here because it's simple and helps me understand the bigger patches....
This one looks good, really just nits.
Jul 13 2018
Restrict matches to prefix/suffix, not substring.
(First foray into clang-tidy, not sure what I'm doing...)
Make strictness (old behavior) available as an option.
Jul 12 2018
Jul 11 2018
I like this idea, of course hard to know how it will affect all practical cases.
Add workaround for formatv+error issues.
As pointed out on D49013, this doesn't work for formatv, as the behavior is different when printing 0/1/2 times.
Landed as r336657, not sure why it didn't get linked to this review.
Jul 10 2018
An alternate approach in D49129: give Error an overloaded operator<< that has the same consume semantics at toString when passed rvalues.
Remove some noise
Make LLVM_[UN]LIKELY top-level
Thanks for looking at this, and sorry it's a complicated space.
Context is: I'm trying to move clangd's logging to wrap formatv instead of ad-hoc concatenation. One of the motivations is to make dealing with llvm::Error/Expected less painful. (In hindsight, we maybe should have rolled our own type here maybe, but Error is pervasive in our code now).
Jul 9 2018
Properly rebase this time, and add/use isASCII in StringExtras.
Rebase and format