Page MenuHomePhabricator

jkorous (Jan Korous)
User

Projects

User does not belong to any projects.

User Details

User Since
Apr 18 2018, 2:22 AM (39 w, 5 d)

Recent Activity

Wed, Jan 16

jkorous added a comment to rCTE351306: [clangd] Fix XPC after rCTE351280.

Thanks a bunch for fixing this Fangrui!

Wed, Jan 16, 11:18 AM

Tue, Jan 15

jkorous committed rCTE351280: [clangd] XPC transport layer.
[clangd] XPC transport layer
Tue, Jan 15, 4:28 PM
jkorous committed rL351280: [clangd] XPC transport layer.
[clangd] XPC transport layer
Tue, Jan 15, 4:28 PM
jkorous closed D54428: [clangd] XPC transport layer, framework, test-client.
Tue, Jan 15, 4:28 PM

Mon, Jan 14

jkorous added a comment to D56610: [clangd] A code action to qualify an unqualified name.

Hi Ilya, I got here from reading your other patch (https://reviews.llvm.org/D56611). I'm wondering if we could make those range utility functions more understandable.

Mon, Jan 14, 4:32 PM
jkorous added a comment to D56611: [clangd] A code action to swap branches of an if statement.

Hi Ilya, this seems really useful for people learning how to implement their custom actions!

Mon, Jan 14, 4:28 PM

Thu, Jan 10

jkorous added a reviewer for D56523: Improve a -Wunguarded-availability note: jkorous.

Hi Erik, this looks neat!

Thu, Jan 10, 4:22 AM

Wed, Jan 9

jkorous added a comment to D54428: [clangd] XPC transport layer, framework, test-client.

@arphaman or @sammccall, could you please take the final look?

Wed, Jan 9, 3:03 AM
jkorous updated the diff for D54428: [clangd] XPC transport layer, framework, test-client.

Fixed the synchronous handling of events.

Wed, Jan 9, 3:03 AM

Tue, Jan 8

jkorous updated the diff for D54428: [clangd] XPC transport layer, framework, test-client.

added one more assert

Tue, Jan 8, 4:10 AM
jkorous awarded D56337: [ADT] Remove attribute LLVM_ALWAYS_INLINE from several classes. a Like token.
Tue, Jan 8, 1:57 AM

Thu, Jan 3

jkorous added a comment to D56226: [clang-format] square parens that are followed by '=' are not Objective-C message sends.

I was thinking the same (that we might be able to make the detection more precise) but I got the impression these cases are handled later (L524 and below).

Thu, Jan 3, 7:04 AM
jkorous added a comment to D55994: [clangd] Check preceding char when completion triggers on ':' or '>'.

@hokein I think you are correct. I meant that it would be possible to make LSP more generic by using trigger strings instead of trigger characters..
@ilya-biryukov Yes, that's true. I would still expect some performance gain in more restrictive filtering on client's side - not sure how big though. Anyway, seems like LSP folks think character + trigger context is good enough. For example here:
https://github.com/Microsoft/language-server-protocol/issues/138

Thu, Jan 3, 2:50 AM
jkorous added a comment to D56226: [clang-format] square parens that are followed by '=' are not Objective-C message sends.

Hi Alex, thanks for fixing this.

Thu, Jan 3, 2:07 AM

Wed, Jan 2

jkorous added a comment to D55994: [clangd] Check preceding char when completion triggers on ':' or '>'.

This looks like a work around LSP imperfection indeed.

Wed, Jan 2, 8:47 AM
jkorous added a comment to D55363: [clangd] Expose FileStatus in LSP..

Maybe consider sending an update after some period of time, e.g. 0.5s? (I expect this particular strategy to be a bit complicated and out of scope of this patch, so another alternative is to simply not send them to the clients in the first version). WDYT?

Sounds fair, keeping status bar blinking seems annony to users. Added a FIXME.

Wed, Jan 2, 7:48 AM

Dec 5 2018

jkorous committed rC348379: [NFC] Use clang-format on PrintingPolicy::PrintingPolicy() after fd5c386f743.
[NFC] Use clang-format on PrintingPolicy::PrintingPolicy() after fd5c386f743
Dec 5 2018, 8:23 AM
jkorous committed rL348379: [NFC] Use clang-format on PrintingPolicy::PrintingPolicy() after fd5c386f743.
[NFC] Use clang-format on PrintingPolicy::PrintingPolicy() after fd5c386f743
Dec 5 2018, 8:23 AM

Nov 29 2018

jkorous added reviewers for D54428: [clangd] XPC transport layer, framework, test-client: ilya-biryukov, ioeric.

Since AFAIK Sam is off until the end of the year I am adding more reviewers.

Nov 29 2018, 9:21 AM
jkorous updated the diff for D54428: [clangd] XPC transport layer, framework, test-client.

Addressed most of the comments.

Nov 29 2018, 9:19 AM

Nov 28 2018

jkorous committed rCTE347738: [clangd] Fix backward-compatibility - follow-up to textDocument/SymbolInfo.
[clangd] Fix backward-compatibility - follow-up to textDocument/SymbolInfo
Nov 28 2018, 2:27 AM
jkorous committed rL347738: [clangd] Fix backward-compatibility - follow-up to textDocument/SymbolInfo.
[clangd] Fix backward-compatibility - follow-up to textDocument/SymbolInfo
Nov 28 2018, 2:27 AM

Nov 27 2018

jkorous committed rCTE347675: [clangd] textDocument/SymbolInfo extension.
[clangd] textDocument/SymbolInfo extension
Nov 27 2018, 8:44 AM
jkorous committed rL347675: [clangd] textDocument/SymbolInfo extension.
[clangd] textDocument/SymbolInfo extension
Nov 27 2018, 8:44 AM
jkorous committed rCTE347674: [clangd][NFC] Move SymbolID to a separate file.
[clangd][NFC] Move SymbolID to a separate file
Nov 27 2018, 8:44 AM
jkorous committed rL347674: [clangd][NFC] Move SymbolID to a separate file.
[clangd][NFC] Move SymbolID to a separate file
Nov 27 2018, 8:44 AM
jkorous closed D54799: [clangd] textDocument/SymbolInfo method.
Nov 27 2018, 8:44 AM
jkorous added a comment to D54799: [clangd] textDocument/SymbolInfo method.

Going to split SymbolID.h as a NFC commit per Alex's suggestion and land this.
Thanks everyone!

Nov 27 2018, 6:58 AM
jkorous retitled D54799: [clangd] textDocument/SymbolInfo method from [clangd][WIP] textDocument/SymbolInfo method to [clangd] textDocument/SymbolInfo method.
Nov 27 2018, 6:56 AM
jkorous updated the diff for D54799: [clangd] textDocument/SymbolInfo method.

Finished tests and fixed operator<<(SymbolDetails) for containerNames that aren't part of fully qualified name.

Nov 27 2018, 6:56 AM
jkorous abandoned D54529: [clangd] Add USR to textDocument/definition response.

Morphed into https://reviews.llvm.org/D54799

Nov 27 2018, 4:43 AM · Restricted Project

Nov 26 2018

jkorous updated the diff for D54799: [clangd] textDocument/SymbolInfo method.

Adressed last comments + initial unit tests.

Nov 26 2018, 12:29 PM
jkorous added inline comments to D54799: [clangd] textDocument/SymbolInfo method.
Nov 26 2018, 12:26 PM
jkorous retitled D54799: [clangd] textDocument/SymbolInfo method from [clangd][WIP] textDocument/CursorInfo method to [clangd][WIP] textDocument/SymbolInfo method.
Nov 26 2018, 5:02 AM
jkorous updated the diff for D54799: [clangd] textDocument/SymbolInfo method.

Removed empty line noise and fixed doxygen annotation.

Nov 26 2018, 5:02 AM
jkorous updated the diff for D54799: [clangd] textDocument/SymbolInfo method.

Addressed comments from the reivew.

Nov 26 2018, 5:01 AM
jkorous added a comment to D54799: [clangd] textDocument/SymbolInfo method.

Thank you very much for the review Sam!

Nov 26 2018, 4:59 AM
jkorous added a comment to D54796: [clangd] C++ API for emitting file status.

Hi @hokein, I am just keeping up to date with changes.

Nov 26 2018, 3:38 AM

Nov 23 2018

jkorous updated the diff for D54799: [clangd] textDocument/SymbolInfo method.

Multiple symbols per location.

Nov 23 2018, 12:30 PM
jkorous updated the diff for D54799: [clangd] textDocument/SymbolInfo method.

Couple minor changes based on discussion.

  • Move SymbolID to index/SymbolID.h.
  • Rename in ClangdServer - drop the verb from method name.
  • Remove conditional return in XRefs.cpp.
Nov 23 2018, 5:14 AM
jkorous added a comment to D54799: [clangd] textDocument/SymbolInfo method.

So I think both SymbolID and USR are optional.

No problem.
I am just wondering if it make sense to include any symbol with empty name, empty USR and no ID in LSP response. I assume either clangd or our LSP client (and hypothetically others too) will have to filter these out. But it might violate consistency with other LSP methods. Anyway, if we agree on filtering such symbols either in getSymbolInfo() or ClangdServer::getSymbolInfo() I am happy to do it, if not it's fine with me.

Nov 23 2018, 4:57 AM

Nov 22 2018

jkorous updated the diff for D54799: [clangd] textDocument/SymbolInfo method.

Changes based on review.

Nov 22 2018, 12:50 PM
jkorous added a comment to D54799: [clangd] textDocument/SymbolInfo method.

Hi Sam.

Nov 22 2018, 12:45 PM

Nov 21 2018

jkorous created D54799: [clangd] textDocument/SymbolInfo method.
Nov 21 2018, 7:24 AM
jkorous planned changes to D54529: [clangd] Add USR to textDocument/definition response.
Nov 21 2018, 6:51 AM · Restricted Project

Nov 19 2018

jkorous added a comment to D54693: [clangd] Store source file hash in IndexFile{In,Out}.

Hi Kadir, I have one small nit otherwise LGTM.

Nov 19 2018, 6:12 AM
jkorous added reviewers for D54047: Check TUScope is valid before use: rjmccall, doug.gregor.

Adding @rjmccall and @doug.gregor who might have some insight.

Nov 19 2018, 3:55 AM

Nov 16 2018

jkorous accepted D53807: Create a diagnostic group for warn_call_to_pure_virtual_member_function_from_ctor_dtor, so it can be turned into an error using Werror.

LGTM. Thanks for the patch!

Nov 16 2018, 9:38 AM
jkorous added a comment to D49736: [Basic] Emit warning flag suggestion only in case there's existing flag *similar* to the unknown one.

Ping.

Nov 16 2018, 5:44 AM
jkorous added a reviewer for D54047: Check TUScope is valid before use: jkorous.
Nov 16 2018, 4:55 AM
jkorous added a comment to D54047: Check TUScope is valid before use.

Could you please add a test? I'd suggest minimizing the testcase you linked and placing it to clang/test.

Nov 16 2018, 4:55 AM
jkorous abandoned D51543: [Sema] Fix uninitialized OverloadCandidate::FoundDecl member.
Nov 16 2018, 3:10 AM · Restricted Project

Nov 15 2018

jkorous planned changes to D54428: [clangd] XPC transport layer, framework, test-client.

A question about the high-level build target setup (I don't know much about XPC services/frameworks, bear with me...):

This is set up so that the clangd binary (ClangdMain) can run unmodified as an XPC service, all flags and options are still respected etc.
At the same time, the framework plist seems like it is designed to invoke the clangd binary in a very specific configuration (no flags will be plumbed through).

Is it actually important that the clangd binary itself can be used with XPC, rather than defining another binary that spawns a ClangdServer+XPCTransport with the right config? If you strip out all of ClangdMain that's not related to flag parsing and options, there's almost nothing left...

Nov 15 2018, 9:50 AM
jkorous added reviewers for D53900: [CodeComplete] Penalize inherited ObjC properties for auto-completion: arphaman, benlangmuir.

This looks reasonable to me but asked people with more context to take a look.

Nov 15 2018, 4:04 AM

Nov 14 2018

jkorous added a comment to D54529: [clangd] Add USR to textDocument/definition response.

Correction - I asked offline about our intended use of USR in LSP and it seems we might want to receive it as part of other responses too. Nothing specific for now but it's probable that it won't be just this singular case.

Nov 14 2018, 10:42 AM · Restricted Project
jkorous added a comment to D54529: [clangd] Add USR to textDocument/definition response.

We are also discussing creating separate method as we'll likely want to add other data to the response in the future. Would you prefer USR not to be in the standard part of LSP but only in our extension?

Nov 14 2018, 9:42 AM · Restricted Project
jkorous added a comment to D54529: [clangd] Add USR to textDocument/definition response.

Hi Sam!

Nov 14 2018, 9:39 AM · Restricted Project
jkorous added a comment to D54529: [clangd] Add USR to textDocument/definition response.

One more thing - shall I create new client capability flag for this?

Nov 14 2018, 8:46 AM · Restricted Project
jkorous created D54529: [clangd] Add USR to textDocument/definition response.
Nov 14 2018, 8:06 AM · Restricted Project

Nov 12 2018

jkorous created D54428: [clangd] XPC transport layer, framework, test-client.
Nov 12 2018, 7:50 AM

Nov 7 2018

jkorous updated the diff for D52554: [WIP] [clangd] Tests for special methods code-completion.

Rewritten tests to shared implementation different cases.

Nov 7 2018, 7:04 AM · Restricted Project
jkorous added inline comments to D52554: [WIP] [clangd] Tests for special methods code-completion.
Nov 7 2018, 7:04 AM · Restricted Project
jkorous accepted D50801: [rename] Use isPointWithin when looking for a declaration at location.

LGTM Thanks for fixing this!

Nov 7 2018, 3:03 AM
jkorous accepted D50926: [SourceManager] Extract 'findFileIDsForFile' from 'translateFile' to allow mapping from one file entry to multiple FileIDs.

LGTM and it seems like all of Eric's comments were answered too.

Nov 7 2018, 2:46 AM

Nov 6 2018

jkorous requested review of D49736: [Basic] Emit warning flag suggestion only in case there's existing flag *similar* to the unknown one.

I tried to move the getNearestOption() to it's only client - EmitUnknownDiagWarning() but it turned out to be a significant change because of clang/Basic/DiagnosticGroups.inc use in DiagnosticIDs.cpp.
I suggest we leave that for eventual future refactoring since it would blow up scope of this patch a lot.

Nov 6 2018, 12:22 PM

Oct 15 2018

jkorous created D53290: [WIP] clangd Transport layer.
Oct 15 2018, 7:42 AM

Sep 26 2018

jkorous created D52554: [WIP] [clangd] Tests for special methods code-completion.
Sep 26 2018, 8:03 AM · Restricted Project

Sep 25 2018

jkorous created D52495: [WIP][Sema][CodeCompletion] Add base type for double colon member completion results.
Sep 25 2018, 7:35 AM

Sep 21 2018

jkorous abandoned D52308: [Sema][CodeCompletion] Fix return type of C++ destructors in code completion.

Allright, I will remove destructor from clangd completion results which solves this particular issue.

Sep 21 2018, 7:06 AM · Restricted Project

Sep 20 2018

jkorous added a comment to D52308: [Sema][CodeCompletion] Fix return type of C++ destructors in code completion.

Sorry my bad. You are right, we aren't showing destructors in clangd for normal classes. The case where I noticed is kind of a corner case with template class.

Sep 20 2018, 9:11 AM · Restricted Project
jkorous added a comment to D52308: [Sema][CodeCompletion] Fix return type of C++ destructors in code completion.

You might be right - I am assuming here that we want consistent behaviour between constructors and destructors.

Sep 20 2018, 8:59 AM · Restricted Project
jkorous created D52308: [Sema][CodeCompletion] Fix return type of C++ destructors in code completion.
Sep 20 2018, 8:38 AM · Restricted Project

Sep 17 2018

jkorous added a comment to D51867: [Diagnostics] Add error handling to FormatDiagnostic().

Hi Volodymyr, do you think you might take another look?

Sep 17 2018, 7:11 AM · Restricted Project

Sep 12 2018

jkorous added a comment to D51488: [Sema][NFC] Small cleanup - remove dead code from ActOnCallExpr() ?.

I got some test failure with the patch, still investigating the issue.

Sep 12 2018, 7:57 AM · Restricted Project

Sep 10 2018

jkorous requested review of D51867: [Diagnostics] Add error handling to FormatDiagnostic().
Sep 10 2018, 2:55 PM · Restricted Project
jkorous added a comment to D51867: [Diagnostics] Add error handling to FormatDiagnostic().

I tried to come up with some input that breaks current implementation so I could add the test. Problem is that invalid memory read doesn't guarantee deterministic crash.
E. g. with this input the current implementation is definitely reading way past the buffer:

SmallVector<char, 1> IgnoreMe;
const char* Foo = "foo%";
const char* FooEnd = Foo + 4;
Diag.FormatDiagnostic(Foo, FooEnd, IgnoreMe);
Sep 10 2018, 2:55 PM · Restricted Project
jkorous retitled D51867: [Diagnostics] Add error handling to FormatDiagnostic() from [Diagnostics][NFC] Add error handling to FormatDiagnostic() to [Diagnostics] Add error handling to FormatDiagnostic().
Sep 10 2018, 11:36 AM · Restricted Project
jkorous planned changes to D51867: [Diagnostics] Add error handling to FormatDiagnostic().

Hi Volodymyr,
Thanks for the feedback - interesting points!

Sep 10 2018, 11:36 AM · Restricted Project
jkorous created D51867: [Diagnostics] Add error handling to FormatDiagnostic().
Sep 10 2018, 10:02 AM · Restricted Project
jkorous added a comment to D51488: [Sema][NFC] Small cleanup - remove dead code from ActOnCallExpr() ?.

Sorry for the delay - I was busy with other things for past two weeks.

Sep 10 2018, 6:54 AM · Restricted Project

Aug 31 2018

jkorous created D51543: [Sema] Fix uninitialized OverloadCandidate::FoundDecl member.
Aug 31 2018, 8:16 AM · Restricted Project

Aug 30 2018

jkorous created D51488: [Sema][NFC] Small cleanup - remove dead code from ActOnCallExpr() ?.
Aug 30 2018, 7:59 AM · Restricted Project
jkorous committed rC341074: [Sema][NFC] Trivial cleanup in ActOnCallExpr.
[Sema][NFC] Trivial cleanup in ActOnCallExpr
Aug 30 2018, 7:48 AM
jkorous committed rL341074: [Sema][NFC] Trivial cleanup in ActOnCallExpr.
[Sema][NFC] Trivial cleanup in ActOnCallExpr
Aug 30 2018, 7:47 AM
jkorous closed D51485: [Sema][NFC] Trivial cleanup in ActOnCallExpr.
Aug 30 2018, 7:47 AM · Restricted Project
jkorous created D51485: [Sema][NFC] Trivial cleanup in ActOnCallExpr.
Aug 30 2018, 4:48 AM · Restricted Project

Aug 16 2018

jkorous added a comment to D50785: [clangd][tests] Add exit(EXIT_FAILURE) in case of JSON parsing failure in TestMode.

Oh, I thought that what everyone wanted was test-specific behaviour. I like both approaches you propose much more!
If we go for the generic option we can effectively start "checking stderr" in tests by using the flag. That would be nice.

Aug 16 2018, 5:52 AM · Restricted Project
jkorous added a comment to D50740: [SourceManager] isPointWithin: avoid using isBeforeInTranslationUnit, compare buffer offsets directly for lexical correctness.

Hi Alex, nice work!

Aug 16 2018, 4:02 AM
jkorous added a reviewer for D50462: Try building complete AST after a fatal error was emitted if further diagnostics are expected: vsapsai.

Adding Volodymyr who landed related patch recently.

Aug 16 2018, 3:30 AM

Aug 15 2018

jkorous created D50785: [clangd][tests] Add exit(EXIT_FAILURE) in case of JSON parsing failure in TestMode.
Aug 15 2018, 9:16 AM · Restricted Project
jkorous committed rL339782: [clangd][tests] Rename tests of clangd instance termination.
[clangd][tests] Rename tests of clangd instance termination
Aug 15 2018, 8:59 AM
jkorous committed rCTE339782: [clangd][tests] Rename tests of clangd instance termination.
[clangd][tests] Rename tests of clangd instance termination
Aug 15 2018, 8:59 AM
jkorous committed rCTE339781: [clangd][tests] Fix typo in tests - invalid LSP exit message.
[clangd][tests] Fix typo in tests - invalid LSP exit message
Aug 15 2018, 8:53 AM
jkorous committed rL339781: [clangd][tests] Fix typo in tests - invalid LSP exit message.
[clangd][tests] Fix typo in tests - invalid LSP exit message
Aug 15 2018, 8:51 AM
jkorous closed D50641: [clangd][test] Fix exit messages in tests.
Aug 15 2018, 8:51 AM · Restricted Project

Aug 14 2018

jkorous added a comment to D50641: [clangd][test] Fix exit messages in tests.

I can see the value of getting more information in case of unexpected test behaviour but I still don't really like to have separate codepath for testing purposes.

Aug 14 2018, 10:42 AM · Restricted Project
jkorous added a comment to D50641: [clangd][test] Fix exit messages in tests.

I think that by introducing different codepath for testing purposes we might end up with fewer bugs in tests but the actual production code could become less tested. Actually, even the -lit-test itself might be not the theoretically most correct approach but I do see the practical reason for it. In general I'd rather keep the testing specific code to a minimum though.

Aug 14 2018, 3:37 AM · Restricted Project

Aug 13 2018

jkorous created D50641: [clangd][test] Fix exit messages in tests.
Aug 13 2018, 9:06 AM · Restricted Project

Aug 10 2018

jkorous planned changes to D49736: [Basic] Emit warning flag suggestion only in case there's existing flag *similar* to the unknown one.
Aug 10 2018, 9:40 AM

Aug 8 2018

jkorous added a comment to D50365: Add a new tool named "lldb-vscode" that implements the Visual Studio Code Debug Adaptor Protocol.

Hi Greg, this looks interesting! I got curious about your patch since I am dealing with another protocol from the same "family" - LSP.

Aug 8 2018, 9:25 AM