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 (97 w, 6 d)

Recent Activity

Today

ilya-biryukov committed rG2754942cbaef: [clangd] Store index in '.clangd/index' instead of '.clangd-index' (authored by ilya-biryukov).
[clangd] Store index in '.clangd/index' instead of '.clangd-index'
Wed, Feb 20, 11:08 AM
ilya-biryukov updated the diff for D58440: [clangd] Store index in '.clangd/index' instead of '.clangd-index'.
  • Update .gitignore
Wed, Feb 20, 11:04 AM · Restricted Project, Restricted Project
ilya-biryukov added inline comments to D58440: [clangd] Store index in '.clangd/index' instead of '.clangd-index'.
Wed, Feb 20, 10:59 AM · Restricted Project, Restricted Project
ilya-biryukov updated the diff for D58440: [clangd] Store index in '.clangd/index' instead of '.clangd-index'.
  • Remove trailing slash from the path
Wed, Feb 20, 10:57 AM · Restricted Project, Restricted Project
ilya-biryukov updated the diff for D58447: [clangd] Fix a crash in Selection.
  • Fix a syntax error in the test
Wed, Feb 20, 6:44 AM · Restricted Project
ilya-biryukov updated the summary of D58447: [clangd] Fix a crash in Selection.
Wed, Feb 20, 6:44 AM · Restricted Project
ilya-biryukov created D58447: [clangd] Fix a crash in Selection.
Wed, Feb 20, 6:43 AM · Restricted Project
ilya-biryukov created D58440: [clangd] Store index in '.clangd/index' instead of '.clangd-index'.
Wed, Feb 20, 4:40 AM · Restricted Project, Restricted Project
ilya-biryukov committed rG97ed3c1e4754: [clangd] Fix a typo. NFC (authored by ilya-biryukov).
[clangd] Fix a typo. NFC
Wed, Feb 20, 4:32 AM
ilya-biryukov added inline comments to rL354444: [clangd] Try to fix windows build bots.
Wed, Feb 20, 3:04 AM

Yesterday

ilya-biryukov accepted D58293: [clang][Index] Enable indexing of Template Type Parameters behind a flag.

LGTM, but see the comment about avoiding code duplication

Tue, Feb 19, 9:29 AM · Restricted Project
ilya-biryukov committed rG93dfb4525675: [clangd] Add an option in the code to not display number of fixes (authored by ilya-biryukov).
[clangd] Add an option in the code to not display number of fixes
Tue, Feb 19, 8:51 AM
ilya-biryukov created D58387: [clangd] Add an option in the code to not display number of fixes.
Tue, Feb 19, 8:13 AM · Restricted Project, Restricted Project
ilya-biryukov accepted D58341: [clangd] Index UsingDecls.

LGTM from my side, with a few NITs.
But let's wait for an LGTM from Haojian too, to make sure his concerns are addressed.

Tue, Feb 19, 7:42 AM · Restricted Project
ilya-biryukov added inline comments to D58293: [clang][Index] Enable indexing of Template Type Parameters behind a flag.
Tue, Feb 19, 7:15 AM · Restricted Project
ilya-biryukov added inline comments to D55250: [clangd] Enhance macro hover to see full definition.
Tue, Feb 19, 4:40 AM · Restricted Project
ilya-biryukov accepted D58294: [clangd] Enable indexing of template type parameters.

LGTM

Tue, Feb 19, 4:37 AM · Restricted Project
ilya-biryukov added a comment to D48116: [libclang] Allow skipping warnings from all included files.

This LGTM wrt to layering, i.e. it's nice we don't need to change the code of diagnostics.
It's been a while since I've looked at the ASTUnit code, though, would be good if someone else took an extra look at it.

Tue, Feb 19, 1:10 AM · Restricted Project
ilya-biryukov added inline comments to D58294: [clangd] Enable indexing of template type parameters.
Tue, Feb 19, 1:00 AM · Restricted Project

Mon, Feb 18

ilya-biryukov added inline comments to D58293: [clang][Index] Enable indexing of Template Type Parameters behind a flag.
Mon, Feb 18, 10:05 AM · Restricted Project
ilya-biryukov added a comment to D58294: [clangd] Enable indexing of template type parameters.

It feels that storing template parameters in the index is a waste, one can only access them through the scope they are introduced in (there are out-of-line definitions of a function, but I think we treat every redeclaration of template headers separately).
Can we get the information from the AST whenever we need it?

Mon, Feb 18, 9:45 AM · Restricted Project
ilya-biryukov added a comment to D58341: [clangd] Index UsingDecls.

std::strcmp is a fair case here. Sema seems not returning using-decls as part of code completion results, it this an intended behavior?

Yeah, I think it is. There's an explicit code path that takes the target decls of a using. Arguably, that's good if you to show signatures of the methods.

Mon, Feb 18, 9:43 AM · Restricted Project
ilya-biryukov added a comment to D58341: [clangd] Index UsingDecls.

For context: @hokein mentioned problems in the clangd's code completion if we would index these symbols.
This patch in itself does not hurt much, users of the indexing API can decide how to deal with UsingDecl on their own, clangd is just one of the clients.

Mon, Feb 18, 7:42 AM · Restricted Project
ilya-biryukov added a comment to D58341: [clangd] Index UsingDecls.

Could we also add a test for code completion to make sure we return the new decls there?

Mon, Feb 18, 6:04 AM · Restricted Project
ilya-biryukov accepted D58340: [clang][Index] Visit UsingDecls and generate USRs for them.

LGTM, but we need to make sure the variable is used in configs without assertions, otherwise we'll break buildbots.

Mon, Feb 18, 5:59 AM · Restricted Project
ilya-biryukov added inline comments to D58340: [clang][Index] Visit UsingDecls and generate USRs for them.
Mon, Feb 18, 5:59 AM · Restricted Project
ilya-biryukov accepted D58190: [clangd] Add tests for template specializations.

LGTM with a typo-nit :-)

Mon, Feb 18, 5:52 AM · Restricted Project, Restricted Project
ilya-biryukov added inline comments to D58190: [clangd] Add tests for template specializations.
Mon, Feb 18, 2:48 AM · Restricted Project, Restricted Project
ilya-biryukov added inline comments to D58340: [clang][Index] Visit UsingDecls and generate USRs for them.
Mon, Feb 18, 2:39 AM · Restricted Project

Fri, Feb 15

ilya-biryukov accepted D58189: [clang][Index] Fix usage of IndexImplicitInstantiation.

LGTM

Fri, Feb 15, 4:39 AM · Restricted Project, Restricted Project

Wed, Feb 13

ilya-biryukov added inline comments to D58189: [clang][Index] Fix usage of IndexImplicitInstantiation.
Wed, Feb 13, 10:36 AM · Restricted Project, Restricted Project
ilya-biryukov accepted D58134: [Analysis] -Wunreachable-code shouldn't fire on the increment of a foreach loop.

LGTM

Wed, Feb 13, 10:13 AM · Restricted Project, Restricted Project
ilya-biryukov accepted D58169: Reapply [VFS] Allow multiple RealFileSystem instances with independent CWDs..

LGTM

Wed, Feb 13, 10:08 AM · Restricted Project
ilya-biryukov added a comment to D58189: [clang][Index] Fix usage of IndexImplicitInstantiation.

Only have a few NITs, will dig deeper into the change tomorrow.
Added @arphaman too as an owner of the index library. Alex, feel free to reassign if you're the wrong person to take a look at this

Wed, Feb 13, 9:58 AM · Restricted Project, Restricted Project
ilya-biryukov added reviewers for D58189: [clang][Index] Fix usage of IndexImplicitInstantiation: ilya-biryukov, arphaman.
Wed, Feb 13, 9:56 AM · Restricted Project, Restricted Project

Tue, Feb 12

ilya-biryukov committed rG6597fdd508f2: [Sema] Fix a crash in access checking for deduction guides (authored by ilya-biryukov).
[Sema] Fix a crash in access checking for deduction guides
Tue, Feb 12, 6:23 AM
ilya-biryukov updated the diff for D58111: [Sema] Fix a crash in access checking for deduction guides.
  • Address comments
Tue, Feb 12, 6:13 AM · Restricted Project
ilya-biryukov created D58111: [Sema] Fix a crash in access checking for deduction guides.
Tue, Feb 12, 2:41 AM · Restricted Project

Mon, Feb 11

ilya-biryukov accepted D58051: Revamp the "[clangd] Format tweak's replacements".

LGTM

Mon, Feb 11, 6:46 AM · Restricted Project
ilya-biryukov planned changes to D57532: [Index] Make sure c-index-test finds libc++ on Mac.

Still need to figure out why the test fails and fix it before submitting the change.

Mon, Feb 11, 1:29 AM · Restricted Project

Fri, Feb 8

ilya-biryukov added a comment to D57739: [clangd] Format tweak's replacements..

It's not about stability or whether the functionality is desired, but layering.
Unit tests having narrow scope is a good thing - if we want system tests that check clangdserver's behavior, they should test clangdserver.
Clients that don't go through clangdserver probably want formatting, but an immediate cleanupAndFormat on each generated change isn't necessarily the right way to do that.

My argument is that it's better to provide formatting by default in the public interface for applying a single tweak.
I might be missing some use-cases, e.g. one that comes to mind is applying multiple tweaks in a row, in which case we'd want to format once and not for every tweak.

Fri, Feb 8, 4:44 AM · Restricted Project
ilya-biryukov added a comment to D57739: [clangd] Format tweak's replacements..

I agree with this concern, and don't think this is an appropriate layer to be aware of styling or failing on format errors. Can we consider moving the formatting to clangdserver as originally planned?

clang-format seems to be somewhat stable, do we actually expect that to be a large problem in practice?
On the other side, I can't imagine any clients that don't need formatting? E.g. it's nice when tests give the same results as one would see in the final clangd and tests don't go through ClangdServer.

Fri, Feb 8, 3:10 AM · Restricted Project

Thu, Feb 7

ilya-biryukov updated the diff for D57532: [Index] Make sure c-index-test finds libc++ on Mac.
  • Simplify code
Thu, Feb 7, 8:02 AM · Restricted Project
ilya-biryukov updated the diff for D57532: [Index] Make sure c-index-test finds libc++ on Mac.
  • Move the string to CIndexer.
Thu, Feb 7, 7:59 AM · Restricted Project
ilya-biryukov accepted D57819: [clangd] Reduce number of threads used by BackgroundIndex to number of physical cores..

LGTM.
NIT about a description: we can't be 100% certain that it's related to hyper-threading, so I'd avoid putting that to the description, maybe simply mention that this avoids higher latency for foreground operations?

Thu, Feb 7, 7:31 AM · Restricted Project
ilya-biryukov added a comment to D57879: [clangd] Fix an assertion failure in Selection..

Repro:

template <class T>
struct Foo {};
Thu, Feb 7, 7:03 AM · Restricted Project, Restricted Project
ilya-biryukov added a reviewer for D57879: [clangd] Fix an assertion failure in Selection.: ilya-biryukov.
Thu, Feb 7, 7:03 AM · Restricted Project, Restricted Project

Wed, Feb 6

ilya-biryukov accepted D57739: [clangd] Format tweak's replacements..

LGTM! Also a few NITs.

Wed, Feb 6, 6:35 AM · Restricted Project
ilya-biryukov committed rG8e42c6224098: [clangd] Update dev dependencies of clangd-vscode (authored by ilya-biryukov).
[clangd] Update dev dependencies of clangd-vscode
Wed, Feb 6, 5:53 AM
ilya-biryukov committed rG7a621551a346: [clangd] Enable clangd on Objective-C in VSCode (authored by ilya-biryukov).
[clangd] Enable clangd on Objective-C in VSCode
Wed, Feb 6, 5:48 AM
ilya-biryukov added a comment to D57814: [clangd] Update dev dependencies of clangd-vscode .

This patch updates dev dependencies, please correct the commit title to avoid confusion :)

Wed, Feb 6, 5:47 AM · Restricted Project, Restricted Project
ilya-biryukov retitled D57814: [clangd] Update dev dependencies of clangd-vscode from [clangd] Update clangd-vscode dependencies to [clangd] Update dev dependencies of clangd-vscode .
Wed, Feb 6, 5:47 AM · Restricted Project, Restricted Project
ilya-biryukov added inline comments to D57739: [clangd] Format tweak's replacements..
Wed, Feb 6, 5:25 AM · Restricted Project
ilya-biryukov added inline comments to D57739: [clangd] Format tweak's replacements..
Wed, Feb 6, 5:08 AM · Restricted Project
ilya-biryukov added a comment to D57814: [clangd] Update dev dependencies of clangd-vscode .

Update the description to add some context

Wed, Feb 6, 3:19 AM · Restricted Project, Restricted Project
ilya-biryukov updated the summary of D57814: [clangd] Update dev dependencies of clangd-vscode .
Wed, Feb 6, 3:19 AM · Restricted Project, Restricted Project
ilya-biryukov created D57814: [clangd] Update dev dependencies of clangd-vscode .
Wed, Feb 6, 3:06 AM · Restricted Project, Restricted Project
ilya-biryukov retitled D57813: [clangd] Enable clangd on Objective-C in VSCode from [clangd] Enable clangd on ObjC in VSCode to [clangd] Enable clangd on Objective-C in VSCode.
Wed, Feb 6, 3:02 AM · Restricted Project
ilya-biryukov created D57813: [clangd] Enable clangd on Objective-C in VSCode.
Wed, Feb 6, 3:01 AM · Restricted Project

Tue, Feb 5

ilya-biryukov added a comment to D57739: [clangd] Format tweak's replacements..

This seems introduce intrusive changes to the Tweak interface (Tweak will need to know the FormatStyle somehow), I think this might be done in another patch.

It's totally fine, since we have just a single tweak now and changing the interface costs nothing.
The important part is making it hard to misuse the interface and forcing formatting on all the users of the interface is a significant burden. E.g. other users of ClangdServer (tests, users of clangd C++ API) shouldn't worry about formatting on their own.

Tue, Feb 5, 9:08 AM · Restricted Project
ilya-biryukov accepted D57755: [clangd] Some minor fixes..

LGTM. Many thanks!

Tue, Feb 5, 8:58 AM · Restricted Project, Restricted Project
ilya-biryukov added inline comments to D57739: [clangd] Format tweak's replacements..
Tue, Feb 5, 3:06 AM · Restricted Project
ilya-biryukov added a comment to D57739: [clangd] Format tweak's replacements..

Could we move the code to the Tweak itself, so that all the callers would get the formatting? I.e.

class Tweak {
   Replacements apply() {  // This is now non-virtual.
     auto Replacements = doApply();
     return cleanupAndFormat(Replacements);
   }
Tue, Feb 5, 3:00 AM · Restricted Project
ilya-biryukov accepted D57581: Explicitly add language standard option to test cases that rely on the C++14 default.

LGTM

Tue, Feb 5, 2:13 AM · Restricted Project, Restricted Project

Fri, Feb 1

ilya-biryukov added a comment to D57581: Explicitly add language standard option to test cases that rely on the C++14 default.

The change looks reasonable to me, given that you can change the default language version via a build argument.
I don't have the required experience with the lit-tests to give proper approval, though, would wait for someone more relevant to do this.

Fri, Feb 1, 6:15 AM · Restricted Project, Restricted Project
ilya-biryukov accepted D57570: [clangd] Expose SelectionTree to code tweaks, and use it for swap if branches..

LGTM.
See the NIT about llvm::, though. Also adding a test with multiple selected ifs would be reasonable, to document and validate behaviour in such cases.

Fri, Feb 1, 5:56 AM · Restricted Project, Restricted Project
ilya-biryukov accepted D57562: [clangd] Lib to compute and represent selection under cursor..

LGTM and sorry for delaying this with discussions.
The change looks solid, regardless of potential problems we may still run into.

Fri, Feb 1, 5:53 AM · Restricted Project, Restricted Project
ilya-biryukov updated the diff for D57573: Disable tidy checks with too many hits.
  • Address comments
Fri, Feb 1, 3:16 AM · Restricted Project, Restricted Project
ilya-biryukov added a comment to D57532: [Index] Make sure c-index-test finds libc++ on Mac.

@arphaman, thanks for pointing this out. It's definitely nice to change stuff only in one place.
There are two issues still pending:

  • index-file.cu still fails, I haven't looked into it closely yet, any idea why that might be happening?; repro is simple, just patch this in locally and run the test.
  • I'm not sure it's safe to use temporary storage for argv[0] in clang_parseTranslationUnit2. The resulting translation units probably holds references to argv and is free to read the data later. A simple and safe alternative would be to store this string in CIndexer and return a reference here. Does that LG?
Fri, Feb 1, 3:09 AM · Restricted Project
ilya-biryukov updated the diff for D57532: [Index] Make sure c-index-test finds libc++ on Mac.
  • Revert "changes to main"
  • Compute the path in parseTranslationUnit2
Fri, Feb 1, 3:05 AM · Restricted Project
ilya-biryukov added inline comments to D57570: [clangd] Expose SelectionTree to code tweaks, and use it for swap if branches..
Fri, Feb 1, 2:52 AM · Restricted Project, Restricted Project
ilya-biryukov added a reviewer for D57573: Disable tidy checks with too many hits: hokein.
Fri, Feb 1, 2:51 AM · Restricted Project, Restricted Project
ilya-biryukov added a comment to D57570: [clangd] Expose SelectionTree to code tweaks, and use it for swap if branches..

Maybe we should consider stopping the traversals up at some points, e.g. at lambda decls or something similar to limit this class of problems in the future?

If you mean in some frameworky sort of way... yeah, it's quite likely we need something. At least to make the Node* -> DynTypedNode -> Stmt -> IfStmt dance less clunky.
I've deliberately avoided adding too much in the way of helpers for now in favor of the raw data structures, mostly wanting to see what the common patterns are - we can afford to write a few of these by hand.

Exactly, the code dealing with typed nodes there is definitely too verbose. Thinking about helpers like findAncestor<IfStmt>().
And yeah, totally fine to keep it as is for now, it's great that the change is focused.

Fri, Feb 1, 2:50 AM · Restricted Project, Restricted Project
ilya-biryukov added a comment to D57562: [clangd] Lib to compute and represent selection under cursor..

The "select everything in a very large file" is exactly the problematic case that came to mind before. I'm sure it's possible to build the selection tree in linear time, but the traversals might also become a problem.

Fri, Feb 1, 2:43 AM · Restricted Project, Restricted Project
ilya-biryukov updated the diff for D57573: Disable tidy checks with too many hits.
  • Also disable 'misc-non-private-member-variables-in-classes' in llvm/.clang-tidy
Fri, Feb 1, 2:28 AM · Restricted Project, Restricted Project
ilya-biryukov created D57573: Disable tidy checks with too many hits.
Fri, Feb 1, 2:24 AM · Restricted Project, Restricted Project
ilya-biryukov added a comment to D57570: [clangd] Expose SelectionTree to code tweaks, and use it for swap if branches..

I wonder if this makes the check available in too many places?

Fri, Feb 1, 1:41 AM · Restricted Project, Restricted Project
ilya-biryukov added a comment to D57562: [clangd] Lib to compute and represent selection under cursor..

Awesome job. Any data on how much time this takes to build? Do you think this would ever be a bottleneck?

Fri, Feb 1, 1:27 AM · Restricted Project, Restricted Project

Thu, Jan 31

ilya-biryukov added a child revision for D56610: [clangd] A code action to qualify an unqualified name: D56612: [clangd] A code action to remove 'using namespace'.
Thu, Jan 31, 1:33 PM · Restricted Project
ilya-biryukov edited parent revisions for D56612: [clangd] A code action to remove 'using namespace', added: 1; removed: 1.
Thu, Jan 31, 1:33 PM
ilya-biryukov removed a child revision for D56611: [clangd] A code action to swap branches of an if statement: D56612: [clangd] A code action to remove 'using namespace'.
Thu, Jan 31, 1:33 PM · Restricted Project, Restricted Project
ilya-biryukov removed a child revision for D56267: [clangd] Interfaces for writing code actions: D56610: [clangd] A code action to qualify an unqualified name.
Thu, Jan 31, 1:32 PM
ilya-biryukov edited parent revisions for D56610: [clangd] A code action to qualify an unqualified name, added: 1; removed: 1.
Thu, Jan 31, 1:32 PM · Restricted Project
ilya-biryukov added a child revision for D56611: [clangd] A code action to swap branches of an if statement: D56610: [clangd] A code action to qualify an unqualified name.
Thu, Jan 31, 1:32 PM · Restricted Project, Restricted Project
ilya-biryukov edited parent revisions for D56611: [clangd] A code action to swap branches of an if statement, added: 1; removed: 1.
Thu, Jan 31, 1:31 PM · Restricted Project, Restricted Project
ilya-biryukov removed a child revision for D56610: [clangd] A code action to qualify an unqualified name: D56611: [clangd] A code action to swap branches of an if statement.
Thu, Jan 31, 1:31 PM · Restricted Project
ilya-biryukov added inline comments to D56611: [clangd] A code action to swap branches of an if statement.
Thu, Jan 31, 1:20 PM · Restricted Project, Restricted Project
ilya-biryukov updated the diff for D56611: [clangd] A code action to swap branches of an if statement.
  • Remove Dummy.cpp
  • Add halfOpenRangeTouches
  • Add a comment about file vs expansion locations
  • Move range manipulations with else and then to apply()
  • Remove test fixture, turn test member functions into free functions
  • Add checkTransform
  • Replace a null check for getCond() with an isValid() check for the corresponding location.
Thu, Jan 31, 1:19 PM · Restricted Project, Restricted Project
ilya-biryukov added a comment to D57532: [Index] Make sure c-index-test finds libc++ on Mac.

I'm not sure who the owners of c-index-test are, so adding people who touched the file recently or might be interested in this landing.
Suggestions for other reviewers are very welcome.

Thu, Jan 31, 11:46 AM · Restricted Project
ilya-biryukov created D57532: [Index] Make sure c-index-test finds libc++ on Mac.
Thu, Jan 31, 11:39 AM · Restricted Project
ilya-biryukov added a comment to D57509: [clangd] Append "(fix available)" to diagnostic message when fixes are present..

I don't think it's pure noise. Vscode displays diagnostics in the "PROBLEMS" tab. A suffix allows you to tell whether fixes are available without hovering on the errors.

And it shows bulb icons when you hover over diagnostics in this dialog too. To be clear, it's my personal preference to not have it, that's the reason I call it 'noise', others might like the message and it feel more useful to them.

Thu, Jan 31, 10:59 AM
ilya-biryukov added a comment to D57509: [clangd] Append "(fix available)" to diagnostic message when fixes are present..

Yes, but a new option seems a bit of an overkill here. The text is appended and doesn't seem to affect the readability of original diagnostic message much (IMO). Could you elaborate why this is undesirable in vscode for you?

It's basically just noise, as VSCode shows the bulb icons whenever you hover over the diagnostics with errors.
Having a standard suffix in half of the diagnostics seems undesirable.

Thu, Jan 31, 10:31 AM
ilya-biryukov accepted D57507: [clang] Add getCommentHandler to PreambleCallbacks.

LGTM, just a few nits

Thu, Jan 31, 10:21 AM · Restricted Project, Restricted Project
ilya-biryukov added a comment to D57509: [clangd] Append "(fix available)" to diagnostic message when fixes are present..

Could this be made optional for VSCode?
As mentioned in the discussion before, I would personally prefer to turn it off, even though I do acknowledge the need for this for some clients, e.g. vim.

Thu, Jan 31, 10:12 AM

Tue, Jan 29

ilya-biryukov created D57384: [clangd] Make -clang-tidy-checks a non-hidden command-line arg.
Tue, Jan 29, 7:13 AM
ilya-biryukov updated the diff for D56612: [clangd] A code action to remove 'using namespace'.
  • Update license header
Tue, Jan 29, 2:19 AM
ilya-biryukov updated the diff for D56611: [clangd] A code action to swap branches of an if statement.
  • Update the license header
Tue, Jan 29, 2:18 AM · Restricted Project, Restricted Project
ilya-biryukov updated the diff for D56610: [clangd] A code action to qualify an unqualified name.
  • Update license header
Tue, Jan 29, 2:18 AM · Restricted Project
ilya-biryukov updated the diff for D56267: [clangd] Interfaces for writing code actions.
  • Update license headers in new files
  • Add an empty cpp file to avoid cmake errors due to empty inputs
  • clang-format
  • Update the 'fixits-command.test' to unbreak it (the line number was larger than the number of lines in the file)
Tue, Jan 29, 2:13 AM
ilya-biryukov added inline comments to D56723: [CodeComplete] Propagate preferred types through parser in more cases.
Tue, Jan 29, 1:54 AM