ilya-biryukov (Ilya Biryukov)
User

Projects

User does not belong to any projects.

User Details

User Since
Apr 6 2017, 1:42 AM (59 w, 9 h)

Recent Activity

Today

ilya-biryukov created D47331: [clangd] Remove accessors for top-level decls from preamble.
Thu, May 24, 9:06 AM
ilya-biryukov added a comment to D47272: [clangd] Build index on preamble changes instead of the AST changes.

Added the test.

Thu, May 24, 8:33 AM
ilya-biryukov updated the diff for D47272: [clangd] Build index on preamble changes instead of the AST changes.
  • Added a test for preamble rebuilds.
Thu, May 24, 8:33 AM
ilya-biryukov added inline comments to D47274: [clangd] Serve comments for headers decls from dynamic index only.
Thu, May 24, 7:47 AM
ilya-biryukov updated the diff for D47274: [clangd] Serve comments for headers decls from dynamic index only.
  • Invert flag, remove the default.
Thu, May 24, 7:47 AM
ilya-biryukov added a comment to D47272: [clangd] Build index on preamble changes instead of the AST changes.

Would it be possible to add some integration tests for file index plus preamble?

Thu, May 24, 7:13 AM
ilya-biryukov added a comment to D41537: Optionally add code completion results for arrow instead of dot.

I have only a few nits left, otherwise looks good. Thanks for making this change! Really excited for it to get in finally!

Thu, May 24, 6:19 AM
ilya-biryukov added a comment to D47223: [clangd] Handle enumerators in named, unscoped enums similarly to scoped enums.

But I feel it's a bit odd that completion and workspace symbols would be inconsistent.

It does not seem that odd to me. Completion is something that should follow the language rules more closely, i.e. we don't want to deviate from sema completions too much. While workspaceSymbol is a user-facing feature and we probably want users to type as little as possible there.
E.g. not showing namespace_name::A in completion seems bad to me, since people are used to referring to the enumerators this way and sema completion gives you those results.

Thu, May 24, 5:50 AM
ilya-biryukov updated the diff for D46943: [clangd] Boost scores for decls from current file in completion.
  • Simplify the change by boosting any Decls that have a redecl in the current file
Thu, May 24, 4:58 AM
ilya-biryukov added a comment to D46943: [clangd] Boost scores for decls from current file in completion.

After an offline chat: we decided to boost sema results that have any decls in the main file. Even though it introduces some unwanted inconsistencies in some cases (e.g. see the comment), most of us agree that's a very simple implementation that does boost useful things, including stuff from association header.
To get better results, we need to design and build the real proximity scoring that handles associated headers and other things.

Thu, May 24, 2:57 AM
ilya-biryukov accepted D47256: [clangd] Fix code completion in MACROs with stringification..

LGTM.

Thu, May 24, 2:10 AM
ilya-biryukov added inline comments to D47063: [clangd] Keep only a limited number of idle ASTs in memory.
Thu, May 24, 2:01 AM
ilya-biryukov updated the diff for D47063: [clangd] Keep only a limited number of idle ASTs in memory.
  • Reimplemented LRU cache with shared_ptr and weak_ptr.
Thu, May 24, 1:52 AM

Yesterday

ilya-biryukov created D47274: [clangd] Serve comments for headers decls from dynamic index only.
Wed, May 23, 11:47 AM
ilya-biryukov updated the diff for D47272: [clangd] Build index on preamble changes instead of the AST changes.
  • Added forgotten renames
Wed, May 23, 11:41 AM
ilya-biryukov added a comment to D47272: [clangd] Build index on preamble changes instead of the AST changes.

@malaperle, a slight offtopic. I was wondering which completion options do you use for clangd? Defaults?

Wed, May 23, 11:37 AM
ilya-biryukov added a comment to D47272: [clangd] Build index on preamble changes instead of the AST changes.

We do not to rely on symbols from the main file anyway, since any info hat those provide can always be taken from the AST.

I'll be adding those soon for workspace symbols... And also for document symbols.

Wed, May 23, 11:35 AM
ilya-biryukov created D47272: [clangd] Build index on preamble changes instead of the AST changes.
Wed, May 23, 11:25 AM
ilya-biryukov added a comment to D46943: [clangd] Boost scores for decls from current file in completion.
  • Symbols store their paths as URIs ⇒ we need to parse them in order to apply heuristics. We could avoid that by writing a version of header-matching that also works on URIs, but that would mean more complexity.

Yeah, this is a bit unfortunate. It's probably OK to parse URIs; they are not that expensive after all (we might want to do some measurement though).

Yeah. I really wish we had microbenchmarks for things like completion.

Wed, May 23, 10:03 AM
ilya-biryukov updated subscribers of D47223: [clangd] Handle enumerators in named, unscoped enums similarly to scoped enums.

I'm not sure if we have tests for that, but I remember that we kept the enumerators in the outer scope so that completion could find them..
Am I right that this patch will change the behavior and we won't get enumerators in the following example:

Wed, May 23, 3:01 AM

Tue, May 22

ilya-biryukov added a comment to D46943: [clangd] Boost scores for decls from current file in completion.

I've added an initial version of testing for the matching header and wanted to get feedback before proceeding further with tests and other changes.

Tue, May 22, 8:00 AM
ilya-biryukov updated the diff for D46943: [clangd] Boost scores for decls from current file in completion.
  • Move helpers tha switch header and source into separate files.
  • Also uprank items coming from the matching headers
Tue, May 22, 7:44 AM
ilya-biryukov added a comment to D47065: [clangd] Enable parsing of non-doxygen comments in global-symbol-builder.

Should we also change the behavior in dynamic index?

Tue, May 22, 3:23 AM
ilya-biryukov added inline comments to D41537: Optionally add code completion results for arrow instead of dot.
Tue, May 22, 2:58 AM
ilya-biryukov added inline comments to D47183: [clangd] Support multiple sema code completion callbacks..
Tue, May 22, 2:30 AM
ilya-biryukov added inline comments to D47183: [clangd] Support multiple sema code completion callbacks..
Tue, May 22, 1:38 AM

Fri, May 18

ilya-biryukov accepted D47068: Move #include manipulation code to new lib/Tooling/Inclusions..

LGTM with a minor naming suggestion.

Fri, May 18, 7:18 AM
ilya-biryukov created D47066: [clangd] Remove ignored Preamble::CanReuse call from completion.
Fri, May 18, 7:10 AM
ilya-biryukov created D47065: [clangd] Enable parsing of non-doxygen comments in global-symbol-builder.
Fri, May 18, 7:05 AM
ilya-biryukov added a comment to D47063: [clangd] Keep only a limited number of idle ASTs in memory.

Another alternative that I've considered was evicting the ASTs from memory after a certain period of time, e.g. after 30 seconds of inactivity for some file. This might be simpler and also cover the use-case of speeding up multiple code navigation requests (findDefinition/documentHighlight) in a row.
Yet another thing that came to mind: walking over all of the AST to find reference under the cursor is terribly inefficient, we should be able to build a cheap data structure that speeds up this operation. Maybe we can even store enough information to not need the AST for navigation at all and build it only for features like refactorings.
@sammccall, let me know what are your thoughts on all of this.

Fri, May 18, 7:03 AM
ilya-biryukov created D47063: [clangd] Keep only a limited number of idle ASTs in memory.
Fri, May 18, 6:57 AM
ilya-biryukov added a comment to D44954: [clangd] Add "member" symbols to the index.

It's probably better to consider this in a future patch. Maybe something like the first suggestion: vector::push_back and match both. Otherwise, I would think it might be a bit too verbose to have to spell out all of the specialization. Maybe we could allow it too. So... all of the above? :)

This certainly does not have to be addressed in this patch. Just wanted to collect opinions on what the behavior we want in the long term.
My thoughts would be towards allowing only vector::push_back and make it match both push_backs: in vector<bool> and vector<T>.
Other cases might work too, but I wouldn't try implementing something that matches specializations. It's just too complicated in the general case. This will only be used in workspaceSymbol and it's fine to give a few more results there...

Fri, May 18, 2:23 AM

Thu, May 17

ilya-biryukov added inline comments to D46943: [clangd] Boost scores for decls from current file in completion.
Thu, May 17, 7:31 AM
ilya-biryukov added inline comments to D46943: [clangd] Boost scores for decls from current file in completion.
Thu, May 17, 5:50 AM
ilya-biryukov updated the diff for D46943: [clangd] Boost scores for decls from current file in completion.
  • Move tests to QualityTests.cpp
  • Fix findDecl() assertion on redecls of the same thing
  • Fix file comment of TestTU.cpp
Thu, May 17, 5:47 AM
ilya-biryukov added inline comments to D46943: [clangd] Boost scores for decls from current file in completion.
Thu, May 17, 4:18 AM
ilya-biryukov added a comment to D44954: [clangd] Add "member" symbols to the index.

A few questions regarding class members. To pinpoint some interesting cases and agree on how we want those to behave in the long run.

Thu, May 17, 2:58 AM
ilya-biryukov added a comment to D41537: Optionally add code completion results for arrow instead of dot.

Sorry, a few more things and we're good to go.

Thu, May 17, 1:52 AM

Wed, May 16

ilya-biryukov created D46943: [clangd] Boost scores for decls from current file in completion.
Wed, May 16, 7:09 AM
ilya-biryukov updated the diff for D45999: [clangd] Retrieve minimally formatted comment text in completion..

Rebase onto head, fix merge conflicts

Wed, May 16, 5:22 AM
ilya-biryukov accepted D46751: [clangd] Filter out private proto symbols in SymbolCollector..

LGTM

Wed, May 16, 5:15 AM
ilya-biryukov added inline comments to D46751: [clangd] Filter out private proto symbols in SymbolCollector..
Wed, May 16, 5:04 AM

Tue, May 15

ilya-biryukov updated the diff for D46002: [clangd] Parse all comments in Sema and completion..
  • Added tests that all comments are being parsed.
Tue, May 15, 8:05 AM
ilya-biryukov added inline comments to D45999: [clangd] Retrieve minimally formatted comment text in completion..
Tue, May 15, 7:36 AM
ilya-biryukov updated the diff for D45999: [clangd] Retrieve minimally formatted comment text in completion..
  • Fix code after a change in deps (getFormattedText now needs a SourceManager instead of an ASTContext)
  • Address review comments
Tue, May 15, 7:27 AM
ilya-biryukov accepted D46524: [clangd] Extract scoring/ranking logic, and shave yaks..

LGTM

Tue, May 15, 7:16 AM
ilya-biryukov updated the diff for D46001: [CodeComplete] Expose helpers to get RawComment of completion result..
  • Rebase onto head and remove \brief from the comments, it's gone upstream now
  • Rename CurrentArg to ArgIndex
Tue, May 15, 6:49 AM
ilya-biryukov added a comment to D41537: Optionally add code completion results for arrow instead of dot.

Maybe move the tests back to this revision?
There are tests for code completion that don't depend on libindex or libclang and use clang -cc1 directly, i.e. tools/clang/test/CodeComplete. This would require adding a flag to clang and patching up PrintingCodeCompleConsumer to print the fix-its, but that should be easy. Moreover, it's useful to have the option to show those completions in clang anyway.

Tue, May 15, 3:22 AM
ilya-biryukov updated the diff for D46000: [AST] Added a helper to extract a user-friendly text of a comment..
  • Removed the overload that accepts ASTContext
Tue, May 15, 1:40 AM

Mon, May 14

ilya-biryukov accepted D46496: [Tooling] Pull #include manipulation code from clangFormat into libToolingCore..

LGTM

Mon, May 14, 8:38 AM
ilya-biryukov added a comment to D46496: [Tooling] Pull #include manipulation code from clangFormat into libToolingCore..

This LG, presuming there are no semantic changes here, just moving the code around.
Also see the nits about IncludeStyle that seems to be in the wrong change...

Mon, May 14, 8:32 AM
ilya-biryukov accepted D46758: [clang-format] Move #include related style to libToolingCore.

LGTM

Mon, May 14, 8:27 AM
ilya-biryukov added a comment to D46758: [clang-format] Move #include related style to libToolingCore.

LG. Just a quick comment about possibly unrelated change.

Mon, May 14, 6:59 AM
ilya-biryukov accepted D45815: [libclang] Allow skipping function bodies in preamble only.

LGTM

Mon, May 14, 6:51 AM
ilya-biryukov added inline comments to D41537: Optionally add code completion results for arrow instead of dot.
Mon, May 14, 6:46 AM
ilya-biryukov added a comment to D41537: Optionally add code completion results for arrow instead of dot.

I hope this review will be over some day :)

Mon, May 14, 5:33 AM
ilya-biryukov updated the diff for D46795: [clangd] Don't query index when completing inside classes.
  • Address review comments
Mon, May 14, 3:52 AM
ilya-biryukov added inline comments to D46795: [clangd] Don't query index when completing inside classes.
Mon, May 14, 3:52 AM

Sat, May 12

ilya-biryukov created D46795: [clangd] Don't query index when completing inside classes.
Sat, May 12, 12:14 PM

Fri, May 11

ilya-biryukov added inline comments to D46758: [clang-format] Move #include related style to libToolingCore.
Fri, May 11, 9:33 AM
ilya-biryukov added a comment to D46751: [clangd] Filter out private proto symbols in SymbolCollector..

Here :)
http://www.sidefx.com/docs/hdk/_s_o_p___bone_capture_lines_8proto_8h_source.html

Didn't take along to find an example. Thanks for digging this up. That looks like a good enough reason to not apply proto-specific filtering based solely on the filename...

Fri, May 11, 9:32 AM
ilya-biryukov added inline comments to D46000: [AST] Added a helper to extract a user-friendly text of a comment..
Fri, May 11, 7:35 AM
ilya-biryukov updated the diff for D46000: [AST] Added a helper to extract a user-friendly text of a comment..
  • Simplify test code with SourceManagerForFile.
Fri, May 11, 7:35 AM
ilya-biryukov added inline comments to D46684: [Frontend] Don't skip function body when the return type is dependent on the template parameter..
Fri, May 11, 7:20 AM
ilya-biryukov accepted D46676: [clangd] Remove LSP command-based #include insertion..

LG when all the dependencies are done

Fri, May 11, 7:10 AM
ilya-biryukov updated subscribers of D46751: [clangd] Filter out private proto symbols in SymbolCollector..

So, the first line of the file generated by proto compiler seems to be something like this:

// Generated by the protocol buffer compiler.  DO NOT EDIT!
Fri, May 11, 7:09 AM
ilya-biryukov added a comment to D46751: [clangd] Filter out private proto symbols in SymbolCollector..

Can there be an option for this? This seems very library specific and could break other code bases. Ideally, there would be a generic mechanism for this kind of filtering, i.e. specify a pattern of excluded files or symbol names. But I understand this would be cumbersome because you want to filter only *some* symbol names in *some* files, so it would be difficult for users to specify this intersection of conditions on command-line arguments, for example. I think this needs to be discussed a bit more or have this turned off by default (with an option to turn on!) until there is a more general solution for this kind of filtering.

Fri, May 11, 7:05 AM
ilya-biryukov added inline comments to D46676: [clangd] Remove LSP command-based #include insertion..
Fri, May 11, 6:57 AM
ilya-biryukov added a comment to D46496: [Tooling] Pull #include manipulation code from clangFormat into libToolingCore..

This change is big enough to be somewhat hard to grasp.
Maybe we could split it into two, to make it easier for review?
I.e.

  • Extraction of IncludeStyle into a separate subclass.
  • Moving stuff around without other refactorings.
Fri, May 11, 6:48 AM
ilya-biryukov added inline comments to D46751: [clangd] Filter out private proto symbols in SymbolCollector..
Fri, May 11, 6:24 AM
ilya-biryukov accepted D46670: [clangd] Move helpers that convert Replacements to TextEdits to SourceCode.h.

LGTM, just a few non-blocking NITs with questions.

Fri, May 11, 2:53 AM
ilya-biryukov added a comment to D46676: [clangd] Remove LSP command-based #include insertion..

This LG, but we should first roll out the additionalEdits change.
Aren't the dependencies reversed in the dependency stack, i.e. this change should be the last one?

Fri, May 11, 2:45 AM

Wed, May 9

ilya-biryukov added a comment to D42966: Fix USR generation in the presence of #line directives or linemarkes.

Hi,

Where do virtual files come from in the first place?

From the linemarker:

Wed, May 9, 10:24 AM
ilya-biryukov created D46639: [CodeComplete] Provide completion in decls even for incomplete types.
Wed, May 9, 7:53 AM
ilya-biryukov updated the diff for D46002: [clangd] Parse all comments in Sema and completion..
  • Moved tests to clang
Wed, May 9, 3:21 AM
ilya-biryukov added inline comments to D45999: [clangd] Retrieve minimally formatted comment text in completion..
Wed, May 9, 2:26 AM
ilya-biryukov updated the diff for D45999: [clangd] Retrieve minimally formatted comment text in completion..
  • Renames and other comments
  • Don't include brief comments in signature help either, comments there are also handled by the code completion code now.
Wed, May 9, 2:17 AM

Tue, May 8

ilya-biryukov added inline comments to D41537: Optionally add code completion results for arrow instead of dot.
Tue, May 8, 8:18 AM
ilya-biryukov updated the diff for D46000: [AST] Added a helper to extract a user-friendly text of a comment..
  • Fixed infinite loop with comments that contain doxygen commands
Tue, May 8, 7:49 AM
ilya-biryukov added a comment to D46001: [CodeComplete] Expose helpers to get RawComment of completion result..

@arphaman, friendly ping. In case you're the wrong person to review it, I'll try to find someone else.

Tue, May 8, 6:46 AM
ilya-biryukov added inline comments to D46000: [AST] Added a helper to extract a user-friendly text of a comment..
Tue, May 8, 6:43 AM
ilya-biryukov updated the diff for D46000: [AST] Added a helper to extract a user-friendly text of a comment..
  • Move unit tests from clangd code to AST tests
  • Assert locations are valid
  • Address review other comments
Tue, May 8, 6:38 AM
ilya-biryukov added a comment to D46524: [clangd] Extract scoring/ranking logic, and shave yaks..

The new uploaded diff has lots of unrelated changes to clang-tidy, clang-move, etc...
Looking at commits, it seems arc diff was called with the wrong base commit...
Could you please reupload the change?

Tue, May 8, 3:05 AM

Mon, May 7

ilya-biryukov added a comment to D46524: [clangd] Extract scoring/ranking logic, and shave yaks..

The change makes both testing and scoring code better.
Even though those are largely independent changes, perfectly happy to review them together.

Mon, May 7, 7:46 AM

Fri, May 4

ilya-biryukov accepted D46180: [clang-format] Refactor #include insertion/deletion functionality into a class..

LGTM

Fri, May 4, 9:34 AM
ilya-biryukov added a comment to D46180: [clang-format] Refactor #include insertion/deletion functionality into a class..

I somehow missed the review email, sorry for the delayed response.

Fri, May 4, 7:38 AM
ilya-biryukov added a comment to D41537: Optionally add code completion results for arrow instead of dot.

Sorry for the delay. I really like the direction of this patch!
What's left is defining the semantics of corrections more thoroughly to make sure we don't have tricky corner cases that users of the API can't deal with.

Fri, May 4, 7:17 AM
ilya-biryukov added a comment to D45815: [libclang] Allow skipping function bodies in preamble only.

Sorry for the delay

Fri, May 4, 6:23 AM
ilya-biryukov added inline comments to D45815: [libclang] Allow skipping function bodies in preamble only.
Fri, May 4, 6:22 AM

Thu, May 3

ilya-biryukov accepted D46183: [clangd] Incorporate #occurrences in scoring code complete results..

LGTM

Thu, May 3, 7:54 AM

Wed, May 2

ilya-biryukov added a comment to D42966: Fix USR generation in the presence of #line directives or linemarkes.

Should we ignore linemarkers and use filename + offset of the real file?

I would say "yes". Let's not rely on linemarkers, unless we can explain why that's a good idea.

Wed, May 2, 6:57 AM
ilya-biryukov added inline comments to D46180: [clang-format] Refactor #include insertion/deletion functionality into a class..
Wed, May 2, 3:12 AM
ilya-biryukov added a comment to D46180: [clang-format] Refactor #include insertion/deletion functionality into a class..

This is really a corner cases that users might not need to know about. But an example is:
Code: "#include <a>" (without \n at the end). After inserting <x>, #include <a>\n#include <x>\n (this is good). However, if you insert another <y>, the code would become "#include <a>\n#include <x>\n\n#include <y>\n" (note the double newline!).

Even though that's rare in practice, it seems like an actual bug.
How hard would be it be to fix it?

Wed, May 2, 2:44 AM

Fri, Apr 27

ilya-biryukov added a comment to D46183: [clangd] Incorporate #occurrences in scoring code complete results..

the patch LG and fixes the problem of llvm namespace not showing up in completions from the static index.
Let's add some tests and ship it!

Fri, Apr 27, 5:51 AM
ilya-biryukov added a comment to D46180: [clang-format] Refactor #include insertion/deletion functionality into a class..

This change LG as an extraction of the helper functionality to be reused in clang, clang-tidy, etc.
However, I feel there are potential improvements both to the underlying code and the new APIs that we could make.

Fri, Apr 27, 3:46 AM

Thu, Apr 26

ilya-biryukov updated subscribers of D42966: Fix USR generation in the presence of #line directives or linemarkes.

The virtual file is actually registered in the SourceManager but the
FileEntry for it is NULL (USRGeneration.cpp:33), which forces printLoc to
return true (USRGeneration.cpp:38) and the USR is not generated.

I still don't get why the file is virtual. Looking at the code in SourceManager, presumed location uses FileEntry of passed location (actually, its expansion location, but that shouldn't matter) and then translates line numbers according to the #line directives in the file.
So the question is: why is FileEntry null for original location, but not null for PresumedLoc?

Thu, Apr 26, 7:52 AM
ilya-biryukov requested changes to D45815: [libclang] Allow skipping function bodies in preamble only.

OK, I've rechecked this change. I don't see any obvious mistake :)

I think I got to the bottom of it. We didn't expect a big win, because we expect people to not put their non-template code into the header files. Which is probably true.
The problem with our reasoning, is that this patch also skip bodies of non-template functions inside template classes, e.g.

Thu, Apr 26, 3:54 AM
ilya-biryukov added a comment to D41537: Optionally add code completion results for arrow instead of dot.

Could you upload the patch with full context?

Thu, Apr 26, 2:50 AM
ilya-biryukov updated the diff for D46000: [AST] Added a helper to extract a user-friendly text of a comment..
  • Remove tryLexCommands(), call into helper that parses commands directly
  • Addressed other review comments
Thu, Apr 26, 2:32 AM
ilya-biryukov added inline comments to D46000: [AST] Added a helper to extract a user-friendly text of a comment..
Thu, Apr 26, 2:32 AM