- User Since
- Apr 6 2017, 1:42 AM (88 w, 6 d)
LGTM. The new code looks simpler and it's arguably simpler for the clients, since they'll have a consistent set of callbacks independent of the actual view into the lambda expression.
The change mostly, the only important comment from me is about toURI. I believe it would actually have an observable effect in the real world and I'm not sure things won't break after it.
Could we avoid changing it and focus only on tweaking the "canonical" path for the FileEntries in this change? Those look like a no-op in most practical cases.
Thanks for the update! We still need to solve the technicality here: could you please upload the diff with full context? See LLVM docs for an example on how this can be done with various version controls (the -U99999 args are important, they are the ones that produce diffs with enough lines around it)
Mon, Dec 17
Update after investigating with @hokein offline: the RecursiveASTVisitor has custom code to visit lambda parameters and return type, but it fallback to visiting typelocs when both parameters and a return type are specified. Unfortunately this fallback does not work when shouldWalkTypeLocs() is set to false, which is the case for our visitor out here.
It seems reasonable to always visit parameters and return type, rather than relying on traversing the full type-loc of the lamda's function type.
Probably the most important suggestion from my side is trying to clearly separate the enqueue calls (which actually schedule rebuilds of TUs) using clang and the loadShards function.
The index loading should be fast, so I assume we won't win much latency by scheduling the indexing actions in the middle of the shard-loading.
OTOH, separating those two concerns should make the code way more readable (I think, at least).
LGTM with a few nits
Good point, we should definitely state it's a clangd-specific extension. We could come up with a naming scheme for our extensions. I would propose something like clangd:$methodName, i.e. the current extensions would be clangd:textDocument/fileStatus.
Thu, Dec 13
- Update the test after changes
Thanks for the review!
- Address review comments
LGTM. When we know everyone is happy with the new behaivour, we can remove the flag altogether.
The RecursiveASTVisitor seems to have the code that properly traverses parameters and return types of lambdas. Any ideas why it's not getting called here?
Sorry if I missed any important design discussions here, but wanted to clear up what information we are trying to convey to the user with the status messages?
E.g. why seeing "building preamble", "building file" or "queued" in the status bar can be useful to the user? Those messages mention internals of clangd, I'm not sure how someone unfamiliar with internals of clangd should interpret this information.
Yay! Anything that makes FileManager and friends simpler LG
LGTM. Many thanks for the fix, really important to get those cases right.
Fri, Dec 7
NIT: I believe we're missing a hyphen in the change description, i.e. it should be disk-backed.
Thu, Dec 6
Maybe try to use Arcanist for uploading diffs?
The web interface can be confusing at times, Arcanist is more reliable (LLVM docs have pointers to the Arcanist documentation to get started)
Wed, Dec 5
Ah, sorry, I incorrectly checked that the old revision landed without problems. Have no data on whether the new revision breaks anything, will have to wait for the input from the US timezone buildcop
It looks like the integrate went smoothly with the new change, thanks for fixing it!
LGTM, still have a few NITs and suggestions (see the comment about emitting "queued" on waiting for diagnostics and not emitting queued if the lock was acquired from the first try), but they don't look like something that should block this change.
- Add a newline to the end of the test file
- Make sure we still run ObjC access checks.
- Add a test with a different NamingClass and BaseType.
- Fix IsSimplyAccessible to work in this case.
D55322 is a review for release notes
Thanks for digging this code up.
I'm a bit confused now, will put up a few clarifying questions to make sure I understand the probem properly.
Does clang-cl work correctly the arguments from compile_commands.json? Does it fail if we add the -resource-dir, similar to how it was done for clangd? The compile-commands.json seems to be generated for the MSVC or hand-crafted, I wonder if it may be a difference between the clang frontend and msvc (unlikely, but just to be sure).
The compile_commands.json does not even mention the -resource-dir, I wonder why would adding it break anything? The only thing that comes to mind is some of the microsoft headers being named the same as the clang headers, but having more stuff in them (the uintptr_t that's missing). Is that the case?
Tue, Dec 4
LGTM with a few NITs.
Also consider adding a test for this.
- Actually use the computed NamingClass
Mon, Dec 3
Sorry for the delays. Not sure about emitting Queued on the main thread, see the corresponding comment. Otherwise looks very good.
Would that option be useful if changing it forces the files user is editing in the IDE to be indefinitely locked and stops them from saving the files?
Anyway, even if you feel this behavior should be configurable, that's totally fine - I was referring to removing the flag from the vfs::FileSystem interface, possibly replacing it with a filesystem implementation that would handle the same thing at a lower level.
This flag is not useful to any of the filesystem implementation, except RealFileSystem, which actually has to deal with opening OS files.
Everything looks good, just a single important request about testing the included files do not have any edges in the resulting graph
- Keep using memory-mapped files for PCHs
- Move once
The idea of using the AST-based approach here was really nice, it was less expensive and seemed to clearly look at the semantic.
I wonder if there's a way to keep it on the AST level, without looking at the source locations.
Thanks for taking a look at this.
It seems there are still cases where memory-mapping should be preferred, even on Windows, specifically:
- PCH files produced by libclang: they are huge, loading them in memory is wasteful and they can clearly be used
- (maybe?) PCM (precompiled module) files: they are huge, however can be produced by the user's buildsystem. Locking them might be bad if we don't control the buildsystem, but the memory savings are attractive.
- other binary input files?
We need to keep mmapping the PCH files.
- Do not introduce a new local var, reuse existing
Fri, Nov 30
I haven't tried QtCreator, but this also happens in VSCode and Vim. I suspect other editors are also affected, but I haven't checked.
I looked at the logs in Process Monitor and I think they do set the share flags properly (~95% certain, will still have to double-check). Even if they do it wrong, it will take time to fix this and I don't think we can afford breaking users with existing editors in our tooling.
https://bugreports.qt.io/browse/QTCREATORBUG-15449 was mentioned previously in the thread.
- Fix a comment
- Add missed field init
- Also fix access checks for member access with qualifiers
@arphaman, did you have a chance to run the tests? There's not rush, just wanted to know whether we have any data at all at how
Thank, LGTM. A few NITs.
LGTM and a few nits
@ilya-biryukov, I hope the current patch is not too big for you to review, happy to chat offline if you want (sam and I had a lot of discussions before he is OOO).
Picking it up. Mostly NITs from my side and a few questions to better understand the design. Also happy to chat offline.
Thu, Nov 29
Yeah, there are still modes where clangd behaves badly there. Note that the output from that pane is rarely useful anyway as clangd keeps producing errors about accessing non-open files, which would confuse people even more.
Overall we never designed this output to be readable by the users, so it's not really useful anyway. There are ways to communicate with the users in the protocol, we should be using them instead to be more user-friendly
+1 to the change, this is annoying for me too.
Wed, Nov 28
That was my initial thought as well and it worked fine on Windows Server 2012. Will have to do it again to double-check, though, not sure I can dig up that code.
I tried calling Windows APIs that LLVM uses with multiple combinations of flags and couldn't make it lock the file. But maybe I didn't arrive at the right combination of flags or was trying on a different version.
cross-posting @zturner's comment from the mailing thread for the record:
I’m afraid this is going to be too severe a performance regression if we change the default. So I don’t think this should be an LLVM-wide default