Page MenuHomePhabricator

ilya-biryukov (Ilya Biryukov)
User

Projects

User does not belong to any projects.

User Details

User Since
Apr 6 2017, 1:42 AM (115 w, 5 d)

Recent Activity

Today

ilya-biryukov added a reviewer for D61681: [clangd] A code tweak to expand a macro: sammccall.
Tue, Jun 25, 1:34 AM · Restricted Project
ilya-biryukov updated the diff for D61681: [clangd] A code tweak to expand a macro.
  • Rebase
  • Update some comments
Tue, Jun 25, 1:34 AM · Restricted Project
ilya-biryukov accepted D63194: [clangd] Link and initialize target infos.

LGTM

Tue, Jun 25, 1:18 AM · Restricted Project
ilya-biryukov accepted D63755: [clang][Tooling] Infer target and mode from argv[0] when using JSONCompilationDatabase.

LGTM

Tue, Jun 25, 1:18 AM · Restricted Project

Yesterday

ilya-biryukov committed rG5e69f27ef708: [Syntax] Do not glue multiple empty PP expansions to a single mapping (authored by ilya-biryukov).
[Syntax] Do not glue multiple empty PP expansions to a single mapping
Mon, Jun 24, 2:41 PM
ilya-biryukov added inline comments to D62953: [Syntax] Do not glue multiple empty PP expansions to a single mapping.
Mon, Jun 24, 9:47 AM · Restricted Project, Restricted Project
ilya-biryukov updated the diff for D62953: [Syntax] Do not glue multiple empty PP expansions to a single mapping.
  • Address comments, document code.
  • s/Expansion/CollectedExpansions.
  • Added FIXMEs for macro arguments.
Mon, Jun 24, 9:47 AM · Restricted Project, Restricted Project
ilya-biryukov added a comment to D63194: [clangd] Link and initialize target infos.

Thanks, this looks very good!
Leaving a few nitpicky comments, but nothing important really.

Mon, Jun 24, 5:41 AM · Restricted Project

Fri, Jun 21

ilya-biryukov added a comment to D63194: [clangd] Link and initialize target infos.

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.

Fri, Jun 21, 7:47 AM · Restricted Project
ilya-biryukov added a comment to D63264: [clang][Driver] Deduce target triplet from clang executable name.

After looking at this, adjusting arguments on the clangd side seems to be a better approach.
Let's abandon this.

Fri, Jun 21, 7:41 AM · Restricted Project
ilya-biryukov added a comment to D61637: [Syntax] Introduce syntax trees.

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.

Fri, Jun 21, 5:08 AM · Restricted Project, Restricted Project
ilya-biryukov added a comment to D61637: [Syntax] Introduce syntax trees.

This is ready for another round now.

Fri, Jun 21, 5:08 AM · Restricted Project, Restricted Project
ilya-biryukov updated the diff for D61637: [Syntax] Introduce syntax trees.
  • A few more renames and docs
  • Cleanups and comments
  • Reformat the code
Fri, Jun 21, 4:58 AM · Restricted Project, Restricted Project
ilya-biryukov accepted D63621: [git-clang-format] recognize hxx as a C++ file.
Fri, Jun 21, 2:17 AM · Restricted Project, Restricted Project
ilya-biryukov accepted D63621: [git-clang-format] recognize hxx as a C++ file.

LGTM.

Fri, Jun 21, 2:17 AM · Restricted Project, Restricted Project

Wed, Jun 19

ilya-biryukov committed rG482269b9fa9d: [clangd] Consume error returned by cleanupAndFormat (authored by ilya-biryukov).
[clangd] Consume error returned by cleanupAndFormat
Wed, Jun 19, 10:28 AM
ilya-biryukov committed rGa7acc7e855ed: [clangd] Format changes produced by rename (authored by ilya-biryukov).
[clangd] Format changes produced by rename
Wed, Jun 19, 10:24 AM
ilya-biryukov created D63562: [clangd] Format changes produced by rename.
Wed, Jun 19, 9:18 AM · Restricted Project, Restricted Project
ilya-biryukov updated the diff for D61637: [Syntax] Introduce syntax trees.
  • Do the renames
Wed, Jun 19, 7:32 AM · Restricted Project, Restricted Project
ilya-biryukov committed rGd0aa6c58beef: [clangd] Collect tokens of main files when building the AST (authored by ilya-biryukov).
[clangd] Collect tokens of main files when building the AST
Wed, Jun 19, 7:01 AM
ilya-biryukov committed rG26c066d66d7a: [Syntax] Fix a crash when dumping empty token buffer (authored by ilya-biryukov).
[Syntax] Fix a crash when dumping empty token buffer
Wed, Jun 19, 6:55 AM
ilya-biryukov added a comment to D62953: [Syntax] Do not glue multiple empty PP expansions to a single mapping.

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.

Wed, Jun 19, 6:26 AM · Restricted Project, Restricted Project
ilya-biryukov updated the diff for D62953: [Syntax] Do not glue multiple empty PP expansions to a single mapping.
  • Address comments
Wed, Jun 19, 3:40 AM · Restricted Project, Restricted Project
ilya-biryukov added inline comments to D62953: [Syntax] Do not glue multiple empty PP expansions to a single mapping.
Wed, Jun 19, 3:40 AM · Restricted Project, Restricted Project

Tue, Jun 18

ilya-biryukov added a comment to D62956: [clangd] Collect tokens of main files when building the AST.

Can we add a test using TestTU that does a very basic verification of expanded/spelled tokens (first after preamble, last token in file)?

Tue, Jun 18, 10:20 AM · Restricted Project, Restricted Project
ilya-biryukov updated the diff for D62956: [clangd] Collect tokens of main files when building the AST.
  • Rename tokens() to getTokens() for consistency with the rest of ParsedAST.
  • Update comments.
  • Add a test.
Tue, Jun 18, 10:19 AM · Restricted Project, Restricted Project
ilya-biryukov added a comment to D62953: [Syntax] Do not glue multiple empty PP expansions to a single mapping.

Can you explain why this is important?
(in the code)

Tue, Jun 18, 9:48 AM · Restricted Project, Restricted Project
ilya-biryukov updated the diff for D62953: [Syntax] Do not glue multiple empty PP expansions to a single mapping.
  • Added comments.
Tue, Jun 18, 9:43 AM · Restricted Project, Restricted Project
ilya-biryukov committed rG5aed309a4f6b: [Syntax] Add a helper to find expansion by its first spelled token (authored by ilya-biryukov).
[Syntax] Add a helper to find expansion by its first spelled token
Tue, Jun 18, 9:32 AM
ilya-biryukov updated the diff for D62954: [Syntax] Add a helper to find expansion by its first spelled token.
  • Added a message to the assertion
Tue, Jun 18, 9:22 AM · Restricted Project, Restricted Project
ilya-biryukov committed rGdf9ee08b649a: [clangd] Return vector<TextEdit> from applyTweak. NFC (authored by ilya-biryukov).
[clangd] Return vector<TextEdit> from applyTweak. NFC
Tue, Jun 18, 8:15 AM
ilya-biryukov added inline comments to D62954: [Syntax] Add a helper to find expansion by its first spelled token.
Tue, Jun 18, 7:58 AM · Restricted Project, Restricted Project
ilya-biryukov updated the diff for D62954: [Syntax] Add a helper to find expansion by its first spelled token.
  • Address comments.
  • s/findExpansion/expansionStartingAt.
  • Add tests.
Tue, Jun 18, 7:58 AM · Restricted Project, Restricted Project
ilya-biryukov accepted D62538: [clangd] Add hidden tweaks to dump AST/selection..

LGTM with a few NITs

Tue, Jun 18, 1:57 AM · Restricted Project, Restricted Project

Thu, Jun 13

ilya-biryukov added a comment to D63194: [clangd] Link and initialize target infos.

Nice, thanks! The clangd parts obviously LGTM, but let's be more conservative with the driver bits (see the relevant comment)

Thu, Jun 13, 5:21 AM · Restricted Project
ilya-biryukov added a comment to D61637: [Syntax] Introduce syntax trees.

This is not 100% ready yet, but wanted to send it out anyway, as I'll be on vacation until Tuesday.

Thu, Jun 13, 5:04 AM · Restricted Project, Restricted Project
ilya-biryukov updated the diff for D61637: [Syntax] Introduce syntax trees.
  • A leaf node stores a single token
  • Restructure code to avoid special-casing leaf nodes
Thu, Jun 13, 4:56 AM · Restricted Project, Restricted Project

Wed, Jun 12

ilya-biryukov added inline comments to D63194: [clangd] Link and initialize target infos.
Wed, Jun 12, 6:55 AM · Restricted Project
ilya-biryukov added inline comments to D63194: [clangd] Link and initialize target infos.
Wed, Jun 12, 6:55 AM · Restricted Project
ilya-biryukov added inline comments to D63194: [clangd] Link and initialize target infos.
Wed, Jun 12, 6:33 AM · Restricted Project
ilya-biryukov committed rG04112ecd41a3: [clangd] Return TextEdits from ClangdServer::applyTweak (authored by ilya-biryukov).
[clangd] Return TextEdits from ClangdServer::applyTweak
Wed, Jun 12, 5:01 AM
ilya-biryukov added a comment to D63140: [clangd] Return TextEdits from ClangdServer::applyTweak.

@hokein just realized you might be a better reviewer, since this makes applyTweak aligned with rename. And you implemented rename in the first place

Wed, Jun 12, 4:15 AM · Restricted Project, Restricted Project
ilya-biryukov added a reviewer for D63140: [clangd] Return TextEdits from ClangdServer::applyTweak: hokein.
Wed, Jun 12, 4:14 AM · Restricted Project, Restricted Project
ilya-biryukov accepted D63193: [clangd] Fix typo in GUARDED_BY().

LGTM

Wed, Jun 12, 3:55 AM · Restricted Project, Restricted Project
ilya-biryukov accepted D63188: Fixed a crash in misc-redundant-expression ClangTidy checker.

LGTM

Wed, Jun 12, 1:36 AM · Restricted Project, Restricted Project

Tue, Jun 11

ilya-biryukov added a comment to D63098: [CodeComplete] Allow completing enum values within case statements, and insert 'case' as a fixit..

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.
Tue, Jun 11, 8:28 AM · Restricted Project
ilya-biryukov added a comment to D62804: [clangd] Enable extraction of system includes from custom toolchains.

For example a gcc cross compiling to arm comes with its own system includes and has some mechanisms to discover that implicitly, without requiring any "-I" flags.
Hence when used with clangd, we make use of system includes instead of the target's include library. This patch aims to solve that issue, tools like cquery also handles that problem in a similar way.

Tue, Jun 11, 7:59 AM · Restricted Project
ilya-biryukov updated the summary of D63140: [clangd] Return TextEdits from ClangdServer::applyTweak.
Tue, Jun 11, 7:36 AM · Restricted Project, Restricted Project
ilya-biryukov created D63140: [clangd] Return TextEdits from ClangdServer::applyTweak.
Tue, Jun 11, 7:31 AM · Restricted Project, Restricted Project
ilya-biryukov accepted D63091: [clangd] Add a capability to enable completions with fixes..

NIT:
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.

Tue, Jun 11, 6:44 AM · Restricted Project, Restricted Project
ilya-biryukov committed rGb37ccc5fece1: [ARM] Fix a typo in the test from r363039 (authored by ilya-biryukov).
[ARM] Fix a typo in the test from r363039
Tue, Jun 11, 6:35 AM
ilya-biryukov committed rG1f6c60270467: Make sure a test from r363036 does not write into a working directory (authored by ilya-biryukov).
Make sure a test from r363036 does not write into a working directory
Tue, Jun 11, 5:04 AM
ilya-biryukov committed rGedea75d6f42e: [Frontend] Avoid creating auxilary files during a unit test. NFC (authored by ilya-biryukov).
[Frontend] Avoid creating auxilary files during a unit test. NFC
Tue, Jun 11, 2:51 AM
ilya-biryukov added a comment to D63098: [CodeComplete] Allow completing enum values within case statements, and insert 'case' as a fixit..

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?

Tue, Jun 11, 2:17 AM · Restricted Project

Fri, Jun 7

ilya-biryukov added inline comments to D62814: [clangd] Treat lambdas as functions when preparing hover response.
Fri, Jun 7, 9:35 AM · Restricted Project, Restricted Project
ilya-biryukov committed rG37e1b41f1b16: AST Matchers tutorial requests to enable clang-tools-extra. NFC (authored by ilya-biryukov).
AST Matchers tutorial requests to enable clang-tools-extra. NFC
Fri, Jun 7, 9:29 AM
ilya-biryukov committed rGa7a1147d4f09: [clangd] Return empty results on spurious completion triggers (authored by ilya-biryukov).
[clangd] Return empty results on spurious completion triggers
Fri, Jun 7, 9:27 AM
ilya-biryukov updated the summary of D62999: [clangd] Return empty results on spurious completion triggers.
Fri, Jun 7, 9:10 AM · Restricted Project, Restricted Project
ilya-biryukov added inline comments to D62999: [clangd] Return empty results on spurious completion triggers.
Fri, Jun 7, 9:09 AM · Restricted Project, Restricted Project
ilya-biryukov updated the diff for D62999: [clangd] Return empty results on spurious completion triggers.
  • Return empty results instead of an error
Fri, Jun 7, 9:09 AM · Restricted Project, Restricted Project
ilya-biryukov retitled D62999: [clangd] Return empty results on spurious completion triggers from [clangd] Return 'RequestCancelled' on spurious completion triggers to [clangd] Return empty results on spurious completion triggers.
Fri, Jun 7, 9:09 AM · Restricted Project, Restricted Project
ilya-biryukov created D62999: [clangd] Return empty results on spurious completion triggers.
Fri, Jun 7, 1:48 AM · Restricted Project, Restricted Project
ilya-biryukov added a comment to D62804: [clangd] Enable extraction of system includes from custom toolchains.

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?

Fri, Jun 7, 1:09 AM · Restricted Project
ilya-biryukov accepted D61697: [lit] Disable test on darwin when building shared libs..

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.

Fri, Jun 7, 12:53 AM · Restricted Project, Restricted Project

Thu, Jun 6

ilya-biryukov committed rG47feb771e139: gn build: Add new tidy checks to gn files (authored by ilya-biryukov).
gn build: Add new tidy checks to gn files
Thu, Jun 6, 7:49 AM
ilya-biryukov updated the summary of D62956: [clangd] Collect tokens of main files when building the AST.
Thu, Jun 6, 7:31 AM · Restricted Project, Restricted Project
ilya-biryukov added parent revisions for D61681: [clangd] A code tweak to expand a macro: D62954: [Syntax] Add a helper to find expansion by its first spelled token, D62953: [Syntax] Do not glue multiple empty PP expansions to a single mapping, D62956: [clangd] Collect tokens of main files when building the AST.
Thu, Jun 6, 7:27 AM · Restricted Project
ilya-biryukov added a child revision for D62954: [Syntax] Add a helper to find expansion by its first spelled token: D61681: [clangd] A code tweak to expand a macro.
Thu, Jun 6, 7:27 AM · Restricted Project, Restricted Project
ilya-biryukov added a child revision for D62953: [Syntax] Do not glue multiple empty PP expansions to a single mapping: D61681: [clangd] A code tweak to expand a macro.
Thu, Jun 6, 7:27 AM · Restricted Project, Restricted Project
ilya-biryukov updated the diff for D61681: [clangd] A code tweak to expand a macro.
  • Move logically separate parts to other changes
Thu, Jun 6, 7:27 AM · Restricted Project
ilya-biryukov added a child revision for D62956: [clangd] Collect tokens of main files when building the AST: D61681: [clangd] A code tweak to expand a macro.
Thu, Jun 6, 7:27 AM · Restricted Project, Restricted Project
ilya-biryukov created D62956: [clangd] Collect tokens of main files when building the AST.
Thu, Jun 6, 7:27 AM · Restricted Project, Restricted Project
ilya-biryukov created D62954: [Syntax] Add a helper to find expansion by its first spelled token.
Thu, Jun 6, 7:24 AM · Restricted Project, Restricted Project
ilya-biryukov created D62953: [Syntax] Do not glue multiple empty PP expansions to a single mapping.
Thu, Jun 6, 7:23 AM · Restricted Project, Restricted Project
ilya-biryukov committed rG54eeb3f40ab1: [clangd] Remove unused signature help quality signal. NFC (authored by ilya-biryukov).
[clangd] Remove unused signature help quality signal. NFC
Thu, Jun 6, 1:31 AM
ilya-biryukov added inline comments to D62476: [clangd] Support offsets for parameters in signatureHelp.
Thu, Jun 6, 1:30 AM · Restricted Project, Restricted Project
ilya-biryukov added inline comments to D62476: [clangd] Support offsets for parameters in signatureHelp.
Thu, Jun 6, 1:19 AM · Restricted Project, Restricted Project
ilya-biryukov committed rG0d02dc60542f: Update AST matchers tutorial to use monorepo layout (authored by ilya-biryukov).
Update AST matchers tutorial to use monorepo layout
Thu, Jun 6, 1:04 AM

Tue, Jun 4

ilya-biryukov accepted D62621: [LibTooling] Add insert/remove convenience functions for creating `ASTEdit`s..

Thanks for the review.

Good questions. The answer to both is that it depends on the RangeSelector used. change itself takes exactly the range given it and modifies the source at that range. Since these APIs are acting at the character level, I think it could be confusing to add token-level reasoning. That said, we could add RangeSelectors and/or TextGenerators that are smarter about this and ensure/add spaces around text as needed. Since all changes should (in my opinion) be run through clang-format afterwards, there's no risk in adding extra whitespace. For example, we could change text() to add whitespace on both sides of the argument, or add a new combinator pad which does that (and leave text() alone). So, for this example, the user would write insertBefore([[FOO]], pad("BAR")) and get BAR FOO.

We could similarly update Stencil to pad its output by default or somesuch.

WDYT?

Tue, Jun 4, 11:25 PM · Restricted Project, Restricted Project
ilya-biryukov committed rGf4302ad35e34: [Syntax] Do not depend on llvm targets for Syntax tests. NFC (authored by ilya-biryukov).
[Syntax] Do not depend on llvm targets for Syntax tests. NFC
Tue, Jun 4, 10:13 AM
ilya-biryukov committed rG2df387b05774: [clangd] Minor cleanup. NFC (authored by ilya-biryukov).
[clangd] Minor cleanup. NFC
Tue, Jun 4, 9:20 AM
ilya-biryukov added a comment to D48116: [libclang] Allow skipping warnings from all included files.

@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.

Tue, Jun 4, 9:08 AM · Restricted Project, Restricted Project
ilya-biryukov added inline comments to D62814: [clangd] Treat lambdas as functions when preparing hover response.
Tue, Jun 4, 8:19 AM · Restricted Project, Restricted Project
ilya-biryukov accepted D62856: [clangd] Also apply adjustArguments when returning fallback commands.

LGTM

Tue, Jun 4, 6:33 AM · Restricted Project, Restricted Project
ilya-biryukov added inline comments to D62814: [clangd] Treat lambdas as functions when preparing hover response.
Tue, Jun 4, 3:35 AM · Restricted Project, Restricted Project
ilya-biryukov requested changes to D62814: [clangd] Treat lambdas as functions when preparing hover response.

Please add a check for the type of variable being not null before landing. The other comment is not very important

Tue, Jun 4, 3:35 AM · Restricted Project, Restricted Project
ilya-biryukov committed rG4ef0f82b71de: [clangd] Support offsets for parameters in signatureHelp (authored by ilya-biryukov).
[clangd] Support offsets for parameters in signatureHelp
Tue, Jun 4, 2:37 AM
ilya-biryukov committed rG30977fc3a97b: [CodeComplete] Include more text into typed chunks of pattern completions (authored by ilya-biryukov).
[CodeComplete] Include more text into typed chunks of pattern completions
Tue, Jun 4, 2:24 AM
ilya-biryukov accepted D62814: [clangd] Treat lambdas as functions when preparing hover response.

Nice catch! I think it makes sense to show signature in those cases as well.
Updating according to that.

Tue, Jun 4, 1:24 AM · Restricted Project, Restricted Project
ilya-biryukov added a comment to D62621: [LibTooling] Add insert/remove convenience functions for creating `ASTEdit`s..

I guess the question also applies to change, although it did not occur to me before.

Tue, Jun 4, 12:41 AM · Restricted Project, Restricted Project
ilya-biryukov added a comment to D62621: [LibTooling] Add insert/remove convenience functions for creating `ASTEdit`s..

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.

Tue, Jun 4, 12:39 AM · Restricted Project, Restricted Project
ilya-biryukov committed rG65de43bc8bee: [clangd] Fix a crash when clang-tidy is disabled (authored by ilya-biryukov).
[clangd] Fix a crash when clang-tidy is disabled
Tue, Jun 4, 12:19 AM

Mon, Jun 3

ilya-biryukov added a comment to D62814: [clangd] Treat lambdas as functions when preparing hover response.

Do we care about pointers or references to lambda types?

Mon, Jun 3, 10:09 AM · Restricted Project, Restricted Project
ilya-biryukov added inline comments to D61637: [Syntax] Introduce syntax trees.
Mon, Jun 3, 10:06 AM · Restricted Project, Restricted Project
ilya-biryukov added a comment to D61637: [Syntax] Introduce syntax trees.

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.

Mon, Jun 3, 9:59 AM · Restricted Project, Restricted Project
ilya-biryukov updated the diff for D61637: [Syntax] Introduce syntax trees.
  • Address comments
Mon, Jun 3, 9:59 AM · Restricted Project, Restricted Project
ilya-biryukov accepted D62582: [CodeComplete] Improve overload handling for C++ qualified and ref-qualified methods..

LGTM

Mon, Jun 3, 2:01 AM · Restricted Project, Restricted Project
ilya-biryukov committed rG1a44584588b7: [CodeComplete] Add a bit more whitespace to completed patterns (authored by ilya-biryukov).
[CodeComplete] Add a bit more whitespace to completed patterns
Mon, Jun 3, 1:33 AM
ilya-biryukov added a comment to D62616: [CodeComplete] Add a bit more whitespace to completed patterns.

I totally agree about the space before the opening curly, but the space before the opening paren is a matter of style. Certainly it does match LLVM style better, but it does not match some other styles.

I guess there's no way to satisfy everyone without an automatic reformatter, and if we were to pick a "default" style it might as well be LLVM, so I approve.

Mon, Jun 3, 1:05 AM · Restricted Project, Restricted Project

Wed, May 29

ilya-biryukov created D62616: [CodeComplete] Add a bit more whitespace to completed patterns.
Wed, May 29, 11:47 AM · Restricted Project, Restricted Project