ilya-biryukov (Ilya Biryukov)
User

Projects

User does not belong to any projects.

User Details

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

Recent Activity

Today

ilya-biryukov added a comment to D43330: [gtest] Add PrintTo overload for StringRef..

Do you think it would be okay to land this if we get no response from @zturner by Monday?
We can revert the commit if any concerns will be raised after that.

Fri, Feb 23, 5:02 AM
ilya-biryukov added inline comments to D43648: [clangd] Debounce streams of updates..
Fri, Feb 23, 2:14 AM

Yesterday

ilya-biryukov accepted D43569: [clangd] Correct setting ignoreWarnings in CodeCompletion..

LGTM with a minor nit.

Thu, Feb 22, 5:07 AM
ilya-biryukov accepted D43518: [clangd] Allow embedders some control over when diagnostics are generated..

LGTM (just one more possibly useful nit about const)

Thu, Feb 22, 5:01 AM
ilya-biryukov added inline comments to D43518: [clangd] Allow embedders some control over when diagnostics are generated..
Thu, Feb 22, 3:56 AM
ilya-biryukov added a comment to D41537: Optionally add code completion results for arrow instead of dot.

Or is your idea is to return the char sequence instead to use this correction in some universal way?

Thu, Feb 22, 3:20 AM
ilya-biryukov accepted D39571: [clangd] DidChangeConfiguration Notification.

Thanks for fixing all the comments! LGTM!

Thu, Feb 22, 2:17 AM

Wed, Feb 21

ilya-biryukov accepted D43550: [clangd] Not collect include headers for dynamic index for now..

As discussed offline, properly supporting IWYU pragma in the dynamic index seems like too much design work for now and it's not gonna get fixed soon.
So opting for disabling the feature seems like the right approach.

Wed, Feb 21, 4:29 AM
ilya-biryukov added a comment to D43550: [clangd] Not collect include headers for dynamic index for now..

Is there a way to still enable include insertion but in a restricted manner, e.g. only for the current project?
File of canonical declaration is usually a pretty good guess and it would be nice to have it. Maybe we could allowing to disable the include insertion via a flag or config parameter rather than disabling it for dynamic index entirely.

Wed, Feb 21, 3:17 AM
ilya-biryukov added inline comments to D39571: [clangd] DidChangeConfiguration Notification.
Wed, Feb 21, 2:45 AM
ilya-biryukov added inline comments to D43518: [clangd] Allow embedders some control over when diagnostics are generated..
Wed, Feb 21, 2:17 AM

Tue, Feb 20

ilya-biryukov added a comment to D41537: Optionally add code completion results for arrow instead of dot.

Sorry for not getting back on this earlier.

Tue, Feb 20, 8:13 AM

Mon, Feb 19

ilya-biryukov accepted D43462: [clangd] Fixes for #include insertion..

LGTM

Mon, Feb 19, 10:38 AM
ilya-biryukov added inline comments to D43462: [clangd] Fixes for #include insertion..
Mon, Feb 19, 10:16 AM
ilya-biryukov added inline comments to D43462: [clangd] Fixes for #include insertion..
Mon, Feb 19, 10:08 AM
ilya-biryukov added inline comments to D43462: [clangd] Fixes for #include insertion..
Mon, Feb 19, 7:41 AM
ilya-biryukov updated the diff for D43377: [clangd] Attach more information about Sema completion to traces.
  • Rename usage of printCompletionKind to getCompletionKindString
  • Remove tracing of the filename, TUScheduler now provides those traces
Mon, Feb 19, 4:25 AM
ilya-biryukov updated the diff for D43379: [CodeComplete] Add a helper to print CodeCompletionContext::Kind.
  • Rename printCompletionKind to getCompletionKindString
Mon, Feb 19, 4:19 AM
ilya-biryukov added inline comments to D43379: [CodeComplete] Add a helper to print CodeCompletionContext::Kind.
Mon, Feb 19, 3:24 AM
ilya-biryukov added a comment to D39571: [clangd] DidChangeConfiguration Notification.

If I do it in this order, I get one diagnostic both times, when I don't expect one the second time the file is parsed. But if I do it the other way (first parse with no errors, second parse with an error), it works fine.

That looks like another preamble-handling bug. It's much easier to fix and it's clangd-specific, so I'll make sure to fix that soon.
Thanks for bringing this up, we haven't been testing compile command changes thoroughly. I'll come up with a fix shortly.

Before this is fixed, you could try writing a similar test with other, non-preprocessor, flags. See ClangdVFSTest.ForceReparseCompileCommand that tests a similar thing by changing -xc to -xc++.
I'll make sure to test the -D case when fixing the bug.

Mon, Feb 19, 3:21 AM
ilya-biryukov created D43454: [clangd] Do not reuse preamble if compile args changed.
Mon, Feb 19, 3:14 AM
ilya-biryukov added a comment to D39571: [clangd] DidChangeConfiguration Notification.

If I do it in this order, I get one diagnostic both times, when I don't expect one the second time the file is parsed. But if I do it the other way (first parse with no errors, second parse with an error), it works fine.

Mon, Feb 19, 2:47 AM
ilya-biryukov accepted D43388: [clangd] Tracing: name worker threads, and enforce naming scheduled async tasks.
Mon, Feb 19, 1:53 AM
ilya-biryukov updated subscribers of D43385: [clangd] Add "clangd.trace" VSCode setting to enable tracing..

LGTM. We'll need to publish new version of VSCode extensions. +@hokein, who did that before.

Mon, Feb 19, 1:41 AM

Fri, Feb 16

ilya-biryukov added inline comments to D43330: [gtest] Add PrintTo overload for StringRef..
Fri, Feb 16, 4:39 AM
ilya-biryukov updated the diff for D43330: [gtest] Add PrintTo overload for StringRef..
  • Assert stream is set in PrintTo()
Fri, Feb 16, 4:39 AM
ilya-biryukov updated the diff for D43246: [clangd] Assert path is absolute when assigning to URIForFile..
  • Rename getFile() to file()
  • Removed the Path.h include
  • Rebased onto head
Fri, Feb 16, 4:20 AM
ilya-biryukov added inline comments to D43246: [clangd] Assert path is absolute when assigning to URIForFile..
Fri, Feb 16, 4:20 AM
ilya-biryukov added a dependency for D43377: [clangd] Attach more information about Sema completion to traces: D43379: [CodeComplete] Add a helper to print CodeCompletionContext::Kind.
Fri, Feb 16, 3:28 AM
ilya-biryukov added a dependent revision for D43379: [CodeComplete] Add a helper to print CodeCompletionContext::Kind: D43377: [clangd] Attach more information about Sema completion to traces.
Fri, Feb 16, 3:28 AM
ilya-biryukov added inline comments to D43377: [clangd] Attach more information about Sema completion to traces.
Fri, Feb 16, 3:28 AM
ilya-biryukov created D43379: [CodeComplete] Add a helper to print CodeCompletionContext::Kind.
Fri, Feb 16, 3:28 AM
ilya-biryukov updated the diff for D43377: [clangd] Attach more information about Sema completion to traces.
  • Attach completion kind in CodeCompleteFlow::run().
  • Move printCompletionKind closer to CompletionContext::Kind.
  • Added FIXME to remove tracing of filename.
Fri, Feb 16, 3:28 AM
ilya-biryukov accepted D35894: [clangd] Code hover for Clangd.

LGTM

Fri, Feb 16, 2:35 AM
ilya-biryukov added a comment to D43377: [clangd] Attach more information about Sema completion to traces.

Technically this is NFC, but it has a huge toString helper and I'm not sure if I chose the appropriate place for logging the filename.

Fri, Feb 16, 2:35 AM
ilya-biryukov created D43377: [clangd] Attach more information about Sema completion to traces.
Fri, Feb 16, 2:31 AM
ilya-biryukov accepted D38639: [clangd] #include statements support for Open definition.
Fri, Feb 16, 2:04 AM

Thu, Feb 15

ilya-biryukov accepted D35894: [clangd] Code hover for Clangd.

Thanks for fixing all the NITs!

Thu, Feb 15, 9:18 AM
ilya-biryukov accepted D38639: [clangd] #include statements support for Open definition.

LGTM modulo the ASSERT_TRUE nit.

Thu, Feb 15, 8:50 AM
ilya-biryukov added a comment to D35894: [clangd] Code hover for Clangd.

Only the naming of fields left.

Thu, Feb 15, 8:41 AM
ilya-biryukov added a comment to D43330: [gtest] Add PrintTo overload for StringRef..

I'd like to hear others have to say about this, (particulary @zturner, as he's the one who came up with the llvm/Testing idea).

Happy to wait for more input on this change.

Thu, Feb 15, 8:05 AM
ilya-biryukov updated the diff for D43330: [gtest] Add PrintTo overload for StringRef..
  • Move PrintTo to gtest-printers.h
Thu, Feb 15, 8:02 AM
ilya-biryukov added a comment to D43229: [clangd] Enable snippet completion based on client capabilities..

As discussed offline with @sammccall, we don't really need to hold Optional<> fields for configuration entries, as we only consume them in clangd and therefore can just set the default values explicitly.
Makes the code simpler and requires only one extra test (with snippets enabled).

Thu, Feb 15, 6:33 AM
ilya-biryukov updated the diff for D43229: [clangd] Enable snippet completion based on client capabilities..
  • Removed completion-snippets-missing.test
Thu, Feb 15, 6:32 AM
ilya-biryukov updated the diff for D43229: [clangd] Enable snippet completion based on client capabilities..
  • Use default values for configuration instead of using Optional<> fields.
  • Removed redundant tests.
Thu, Feb 15, 6:32 AM
ilya-biryukov added inline comments to D43229: [clangd] Enable snippet completion based on client capabilities..
Thu, Feb 15, 5:32 AM
ilya-biryukov updated the diff for D43227: [clangd] Make functions of ClangdServer callback-based.
  • Rebase onto head
Thu, Feb 15, 4:51 AM
ilya-biryukov added inline comments to D43229: [clangd] Enable snippet completion based on client capabilities..
Thu, Feb 15, 4:40 AM
ilya-biryukov updated the diff for D43229: [clangd] Enable snippet completion based on client capabilities..
  • Remove initializers from Optional<> fields
Thu, Feb 15, 4:39 AM
ilya-biryukov added a comment to D43330: [gtest] Add PrintTo overload for StringRef..

This code does not have to live directly inside gtest. As such, maybe a better place for it would be somewhere under include/llvm/Testing ? (We already have PrintTo functions for Error and Expected there..)

The reason I'm mentioning this is that while StringRef is a fairly basic class, I'm sure other classes would benefit from being GTEST-ified as well, and shoving them all somewhere into third party code sounds like a bad idea...

Thu, Feb 15, 4:16 AM
ilya-biryukov added a comment to D35894: [clangd] Code hover for Clangd.

Just a few last remarks and this is good to go.
Should I land it for you after the last comments are fixed?

Thu, Feb 15, 4:01 AM
ilya-biryukov created D43330: [gtest] Add PrintTo overload for StringRef..
Thu, Feb 15, 2:48 AM
ilya-biryukov added a comment to D38639: [clangd] #include statements support for Open definition.

Last drop of NITs. After they're fixed, this change is ready to land!

Thu, Feb 15, 1:48 AM

Wed, Feb 14

ilya-biryukov added inline comments to D38639: [clangd] #include statements support for Open definition.
Wed, Feb 14, 5:37 AM
ilya-biryukov accepted D43272: [clangd] Fix tracing now that spans lifetimes can overlap on a thread..

LG with a few NITs.

Wed, Feb 14, 5:28 AM
ilya-biryukov added a comment to D39571: [clangd] DidChangeConfiguration Notification.
It seems to me like

changes in command line arguments (adding -DMACRO=1) are not taken into account
when changing configuration.

Wed, Feb 14, 4:35 AM
ilya-biryukov added a comment to D43246: [clangd] Assert path is absolute when assigning to URIForFile..

I think another option to prevent the bug in r325029 from happening would be providing a canonical approach for retrieving (absolute) paths from FileManager. We already have code in symbol collector that does this, and we have similar code in XRefs.cpp now. We should probably move them to clangd/Path.h and share the code?

Wed, Feb 14, 3:53 AM
ilya-biryukov updated the diff for D43246: [clangd] Assert path is absolute when assigning to URIForFile..
  • Remove URIForFile::setFile(), rely on copy and move constructors instead.
Wed, Feb 14, 3:51 AM
ilya-biryukov added a comment to D43230: [clangd] Explicitly initialize all primitive fields in Protocol.h.

I think it would probably depend on whether we could find a sensible default value for a field (e.g. is 0 a better default value than UINT_MAX for line number?) and whether we expect users to always initialize a field (e.g. would it be better to pollute the field instead of providing a default value so that uninitialized values would be caught more easily).

Yeah, it arguable whether 0 is the right default, but at least it's consistent with what compiler uses for zero-initialization, i.e. when you default-initialize builtin types explicitly (e.g. int b = int(); or int b = *(new int)).
For things like FileChangeType we choose 1, because it's the lowest acceptable value, but I'm not sure that's the right default value, merely a random valid enum value.

Wed, Feb 14, 2:51 AM
ilya-biryukov updated the diff for D43230: [clangd] Explicitly initialize all primitive fields in Protocol.h.
  • Removed initializers for Optional<> fields, they are not problematic
  • Remove ctor from Position, initialize on per-field basis instead.
Wed, Feb 14, 2:41 AM
ilya-biryukov added a comment to D43227: [clangd] Make functions of ClangdServer callback-based.

I wonder whether we want to introduce using Callback<T...> = UniqueFunction<void(T...)> for readability at some point...

I agree that's a good idea, will come up with a CL doing that.

Wed, Feb 14, 1:36 AM
ilya-biryukov updated the diff for D43227: [clangd] Make functions of ClangdServer callback-based.
  • Use llvm::Optional<> in capture() helper to avoid confusion with llvm::Expected.
Wed, Feb 14, 1:36 AM

Tue, Feb 13

ilya-biryukov updated the diff for D43227: [clangd] Make functions of ClangdServer callback-based.
  • Capture only needed vars in lambda, not everything.
  • Rebase onto head.
Tue, Feb 13, 10:37 AM
ilya-biryukov added inline comments to D43227: [clangd] Make functions of ClangdServer callback-based.
Tue, Feb 13, 10:37 AM
ilya-biryukov added a comment to D35894: [clangd] Code hover for Clangd.

Is there a way to get the macro name from the MacroInfo object? I couldn't find any, so the solution I'm considering is to make DeclarationAndMacrosFinder::takeMacroInfos return an std::vector<std::pair<StringRef, const MacroInfo *>>, where the first member of the pair is the macro name. It would come from IdentifierInfo->getName() in DeclarationAndMacrosFinder::finish. Does that make sense, or is there a simpler way?

Tue, Feb 13, 10:10 AM
ilya-biryukov added a comment to D43230: [clangd] Explicitly initialize all primitive fields in Protocol.h.

But I think it's safe and probably easier to rely on default values of primitive types like int, bool etc

It's not always safe, as primitive types are sometimes left uninitialized (e.g. when constructed on the stack) and reading an uninitialized value is UB.

Tue, Feb 13, 10:06 AM
ilya-biryukov added inline comments to D43230: [clangd] Explicitly initialize all primitive fields in Protocol.h.
Tue, Feb 13, 10:00 AM
ilya-biryukov created D43246: [clangd] Assert path is absolute when assigning to URIForFile..
Tue, Feb 13, 9:58 AM
ilya-biryukov added a comment to D39571: [clangd] DidChangeConfiguration Notification.

I haven't looked at the newest patch yet but I gave it a quick try and saw something odd. If I change the configuration to something invalid (say I specify the path to a CMakeLists.txt), then I get many errors/diagnostics, which is normal. But then when I change the config to something valid, the same diagnostics re-emitted, as if the configuration failed to change. I'll check the code tomorrow a bit but I thought I'd share this with you early.

Tue, Feb 13, 6:26 AM
ilya-biryukov added inline comments to D38639: [clangd] #include statements support for Open definition.
Tue, Feb 13, 6:08 AM
ilya-biryukov added a comment to D43230: [clangd] Explicitly initialize all primitive fields in Protocol.h.

This commit makes potential UB less likely. Logical errors (i.e. forgot to initialize some fields) will still be there.
Sending this for review to make sure everyone agrees this is a good thing.

Tue, Feb 13, 3:42 AM
ilya-biryukov updated the diff for D43230: [clangd] Explicitly initialize all primitive fields in Protocol.h.
  • Removed initializers for Optional<> fields, they are not problematic
Tue, Feb 13, 3:42 AM
ilya-biryukov created D43230: [clangd] Explicitly initialize all primitive fields in Protocol.h.
Tue, Feb 13, 3:37 AM
ilya-biryukov created D43229: [clangd] Enable snippet completion based on client capabilities..
Tue, Feb 13, 3:25 AM
ilya-biryukov created D43227: [clangd] Make functions of ClangdServer callback-based.
Tue, Feb 13, 2:29 AM
ilya-biryukov accepted D43127: [clangd] Stop exposing Futures from ClangdServer operations..

LGTM

Tue, Feb 13, 12:15 AM

Mon, Feb 12

ilya-biryukov added a comment to D35894: [clangd] Code hover for Clangd.

The only problem I see is that when hovering a function/struct name, it now prints the whole function/struct definition. When talking with @malaperle, he told me that you had discussed it before and we should not have the definition in the hover, just the prototype for a function and the name (e.g. "struct foo") for a struct. Glancing quickly at the PrintingPolicy, I don't see a way to do that. Do you have any idea?

Arrg, Sorry, I just re-read your comment and saw that you used TerseOutput which does that. I did not catch that when transcribing the code (I did not want to copy paste to understand the code better, but got bitten).

Mon, Feb 12, 8:59 AM
ilya-biryukov added inline comments to D43127: [clangd] Stop exposing Futures from ClangdServer operations..
Mon, Feb 12, 7:38 AM
ilya-biryukov added inline comments to D43127: [clangd] Stop exposing Futures from ClangdServer operations..
Mon, Feb 12, 4:47 AM
ilya-biryukov added inline comments to D43123: [clangd] Log all ignored diagnostics..
Mon, Feb 12, 3:47 AM
ilya-biryukov updated the diff for D43123: [clangd] Log all ignored diagnostics..
  • Renamed logIgnoredDiag to log.
Mon, Feb 12, 3:47 AM
ilya-biryukov added inline comments to D43068: [clangd] Remove codeComplete that returns std::future<>.
Mon, Feb 12, 3:40 AM
ilya-biryukov updated the diff for D43068: [clangd] Remove codeComplete that returns std::future<>.
  • Added CaptureProxy<T> helper, use it to implement runCodeComplete.
  • Documented that we deliberately don't expose the sync API in tests.
  • Rebased onto the latest head.
Mon, Feb 12, 3:38 AM
ilya-biryukov accepted D42895: [libclang] Add `CXSymbolRole role` to CXIdxEntityRefInfo.

LGTM.

Is it possible to see this landed before clang+llvm 6 is released?

I don't know in detail how releases work, but I believe the release branch has already been created and this patch will need to be merged into it separately (i.e. landing this upstream won't get it into the release automatically).
But again, I'm not sure how releases work. Asking on cfe-dev or llvm-dev should be your best bet.

Mon, Feb 12, 2:12 AM
ilya-biryukov accepted D43091: [gtest] Support raw_ostream printing functions more comprehensively..
Mon, Feb 12, 1:52 AM

Fri, Feb 9

ilya-biryukov updated the diff for D43122: [clangd] Fix crash when CompilerInvocation can't be created..
  • Removed assert that is now redundant
  • Moved EXPECT_ERROR to Matchers.h
  • Added more context into EXPECT_ERROR's error message
Fri, Feb 9, 5:52 AM
ilya-biryukov created D43123: [clangd] Log all ignored diagnostics..
Fri, Feb 9, 5:29 AM
ilya-biryukov created D43122: [clangd] Fix crash when CompilerInvocation can't be created..
Fri, Feb 9, 5:20 AM
ilya-biryukov added inline comments to D42895: [libclang] Add `CXSymbolRole role` to CXIdxEntityRefInfo.
Fri, Feb 9, 1:42 AM

Thu, Feb 8

ilya-biryukov added inline comments to D43068: [clangd] Remove codeComplete that returns std::future<>.
Thu, Feb 8, 10:08 AM
ilya-biryukov added inline comments to D43068: [clangd] Remove codeComplete that returns std::future<>.
Thu, Feb 8, 7:38 AM
ilya-biryukov updated the diff for D43065: [clangd] Remove threading-related code from ClangdUnit.h.
  • Removed braces
  • s/latest/last/
Thu, Feb 8, 6:57 AM
ilya-biryukov added a comment to D43065: [clangd] Remove threading-related code from ClangdUnit.h.

Thanks for the NITs :-)

Thu, Feb 8, 6:57 AM
ilya-biryukov added a comment to D35894: [clangd] Code hover for Clangd.

Thanks for picking this up!
I have just one major comment related to how we generate the hover text.

Thu, Feb 8, 6:43 AM
ilya-biryukov created D43068: [clangd] Remove codeComplete that returns std::future<>.
Thu, Feb 8, 4:46 AM
ilya-biryukov added inline comments to D42895: [libclang] Add `CXSymbolRole role` to CXIdxEntityRefInfo.
Thu, Feb 8, 3:39 AM
ilya-biryukov created D43065: [clangd] Remove threading-related code from ClangdUnit.h.
Thu, Feb 8, 1:57 AM

Wed, Feb 7

ilya-biryukov added inline comments to D42895: [libclang] Add `CXSymbolRole role` to CXIdxEntityRefInfo.
Wed, Feb 7, 9:36 AM
ilya-biryukov accepted D43009: [clangd] Do not precent-encode numbers in URI..
Wed, Feb 7, 4:11 AM

Tue, Feb 6

ilya-biryukov updated the diff for D42573: [clangd] The new threading implementation.
  • Addressed the last review comment
Tue, Feb 6, 7:54 AM