- User Since
- Apr 6 2017, 1:42 AM (115 w, 5 d)
- Update some comments
- Address comments, document code.
- Added FIXMEs for macro arguments.
Thanks, this looks very good!
Leaving a few nitpicky comments, but nothing important really.
Fri, Jun 21
Sorry about the confusion, I can now see why the original version of the patch was actually simpler.
I was put off by the fact that we override by adding command-line arguments instead of passing things around in the code, but it appears that's actually simpler than adding yet another mechanism to override target tripple in the driver.
After looking at this, adjusting arguments on the clangd side seems to be a better approach.
Let's abandon this.
Although there are still rough edges, I believe the storage model is agreed upon and we can hopefully address the rest in the follow-ups.
This is ready for another round now.
- A few more renames and docs
- Cleanups and comments
- Reformat the code
Wed, Jun 19
- Do the renames
To clarify, I don't think there are new concepts in this patch.
Previously, we only took information from source locations into account when building token buffers. That works fine in most cases, but not enough to find the boundaries of empty macro expansions.
In order to find those, we now also watch the callbacks from the preprocessor to get all macro expansions (incuding the empty ones) and reconstruct the mappings of token buffers from there.
- Address comments
Tue, Jun 18
- Rename tokens() to getTokens() for consistency with the rest of ParsedAST.
- Update comments.
- Add a test.
- Added comments.
- Added a message to the assertion
- Address comments.
- Add tests.
LGTM with a few NITs
Thu, Jun 13
Nice, thanks! The clangd parts obviously LGTM, but let's be more conservative with the driver bits (see the relevant comment)
This is not 100% ready yet, but wanted to send it out anyway, as I'll be on vacation until Tuesday.
- A leaf node stores a single token
- Restructure code to avoid special-casing leaf nodes
Wed, Jun 12
@hokein just realized you might be a better reviewer, since this makes applyTweak aligned with rename. And you implemented rename in the first place
Tue, Jun 11
Notes from the offline discussion:
- Sam: supporting multiple typed text chunks is hard, many clients that expect only one. Objective-C already does that, though, but in a way that does not break most targets.
- Sam: supporting any other form of custom text for filtering is also hard, particularly in clangd that exposes a structured completion item with qualifiers, etc.
- me: feel strongly that we should show case at the start and match it.
- me: also feel strongly that the feature is useful. Having a single big "typed text" chunk that includes the qualifier and 'case' looks better than not having the feature.
I've been pushing towards giving clangd-specific extensions names that would absolutely never clash with anything that LSP proposes. In particular, the file status notification is textDocument/clangd.fileStatus.
With that in line, I propose to use clangd.editsNearCursor for the flag name. Other than that, LGTM.
Not displaying 'case' is actually confusing to my taste.
WDYT about allowing multiple "typed text" chunks or allowing to mark other chunks that are supposed be part of filter text?
Fri, Jun 7
- Return empty results instead of an error
Could you give more context on what the custom toolchains are?
One feasible alternative is to move this detection to clang's driver (where the rest of include path detection lives), why won't that work?
LGTM, I see no way around it, we can't copy all of the shared libs.
Maybe there's an alternative way to test this without copying the binary, but I don't have one in mind.
Thu, Jun 6
- Move logically separate parts to other changes
Tue, Jun 4
@jkorous, could you take a look or suggest someone who can review the libclang changes?
As mentioned before, I've reviewed the clang bits (which are now gone), but don't have much experience in libclang parts.
Please add a check for the type of variable being not null before landing. The other comment is not very important
I guess the question also applies to change, although it did not occur to me before.
Are there any corner cases with handling whitespace that we might want to handle or document them if we don't handle them?
E.g. if the range ends up being a name of an expanded macro , it's probably very easy to glue the macro name with whatever you're inserting.
Mon, Jun 3
Do we care about pointers or references to lambda types?
I've addressed most of the comments, except the naming ones.
We need a convention for naming the language nodes and names for composite and leaf structural nodes.
- Address comments