Page MenuHomePhabricator

hokein (Haojian Wu)
User

Projects

User does not belong to any projects.

User Details

User Since
Aug 19 2015, 3:38 AM (183 w, 9 h)

Recent Activity

Today

hokein added inline comments to D58345: [clangd] Using symbol name to map includes for STL symbols..
Wed, Feb 20, 11:10 AM · Restricted Project

Mon, Feb 18

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

I wonder how does merge work with Sema results? See the case below, IIUC our indexer has one symbol for this using decl, but the code completion result from Sema contains two symbols. The symbol ids of these 3 symbols are different, so we will end up with 3 completion results.

That's true, but we're not sure how much this would hurt in practice. Currently we don't show any results from dynamic index for std::strcmp, which is arguably worse than showing an extra completion item for the using.

Mon, Feb 18, 8:11 AM · Restricted Project
hokein added a comment to D58345: [clangd] Using symbol name to map includes for STL symbols..

Looks great! Thanks for doing this.

Could you also check in the tool that is used to generate the mapping? We need a way to update the mapping when cppreference is updated.

Mon, Feb 18, 7:44 AM · Restricted Project
hokein accepted D58275: [clangd] Support utf-8 offsets (rather than utf-16) as a protocol extension.

I think we need to update the comment in the SymbolLocation::Column.

Mon, Feb 18, 7:33 AM · Restricted Project
hokein added a comment to D58341: [clangd] Index UsingDecls.

I wonder how does merge work with Sema results? See the case below, IIUC our indexer has one symbol for this using decl, but the code completion result from Sema contains two symbols. The symbol ids of these 3 symbols are different, so we will end up with 3 completion results.

Mon, Feb 18, 7:27 AM · Restricted Project
hokein updated the diff for D58345: [clangd] Using symbol name to map includes for STL symbols..

Fix style.

Mon, Feb 18, 4:53 AM · Restricted Project
hokein created D58345: [clangd] Using symbol name to map includes for STL symbols..
Mon, Feb 18, 4:52 AM · Restricted Project

Fri, Feb 15

hokein added a comment to D58275: [clangd] Support utf-8 offsets (rather than utf-16) as a protocol extension.

The code looks good. For this protocol extension, we need supports from other LSP clients, I think we may want to propose this extension to the LSP specification, so that all LSP servers/clients respect it.

Fri, Feb 15, 3:21 AM · Restricted Project

Mon, Feb 11

hokein added a comment to D57943: [clangd] **Prototype**: clang-tidy-based tweaks.

This is an intriguing idea, and is at least useful to prototype new tweaks.

I'm not sure whether clang-tidy is the ultimately right API to write tweaks:

it doesn't have the needed constraints to ensure prepare() is fast (e.g. it emits diagnostics and fixes eagerly)
the exact set of nodes that it will trigger on may or may not match what we want
it doesn't produce context-sensitive titles

Mon, Feb 11, 7:49 AM · Restricted Project
hokein updated the diff for D57943: [clangd] **Prototype**: clang-tidy-based tweaks.

Update the comments

Mon, Feb 11, 7:49 AM · Restricted Project
hokein committed rGe64ee7c6458b: Revamp the "[clangd] Format tweak's replacements" (authored by hokein).
Revamp the "[clangd] Format tweak's replacements"
Mon, Feb 11, 7:18 AM
hokein committed rL353712: Revamp the "[clangd] Format tweak's replacements".
Revamp the "[clangd] Format tweak's replacements"
Mon, Feb 11, 7:18 AM
hokein committed rCTE353712: Revamp the "[clangd] Format tweak's replacements".
Revamp the "[clangd] Format tweak's replacements"
Mon, Feb 11, 7:18 AM
hokein closed D58051: Revamp the "[clangd] Format tweak's replacements".
Mon, Feb 11, 7:18 AM · Restricted Project
hokein updated the diff for D58051: Revamp the "[clangd] Format tweak's replacements".

Address review comment.

Mon, Feb 11, 7:00 AM · Restricted Project
hokein added reviewers for D58051: Revamp the "[clangd] Format tweak's replacements": sammccall, ilya-biryukov.
Mon, Feb 11, 5:47 AM · Restricted Project
hokein retitled D58051: Revamp the "[clangd] Format tweak's replacements" from Revert "[clangd] Format tweak's replacements." to Revamp the "[clangd] Format tweak's replacements".
Mon, Feb 11, 5:47 AM · Restricted Project
hokein updated the diff for D58051: Revamp the "[clangd] Format tweak's replacements".
  • [clangd] Re-submit format replacement for tweaks.
Mon, Feb 11, 5:44 AM · Restricted Project
hokein created D58051: Revamp the "[clangd] Format tweak's replacements".
Mon, Feb 11, 5:42 AM · Restricted Project
hokein 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.

If providing formatting was free, I'd be fine with this, but it leaks into the public interface in two places: a) it requires the caller to plumb through a formatting style, and b) it is another source of errors.

For this particular interface it's more important that it's conceptually clear and easy to implement than it is that it's easy to call - this is an extension point.

My concerns are code duplication and ease of use for the clients. Having both apply and applyAndFormat (the latter being a non-virtual or a free-standing utility function) in the public interface of the Tweak would totally do it for me.

I'd be happy with applyAndFormat as a free function - my main concern is that formatting not be part of the scope of the class.

Mon, Feb 11, 2:41 AM · Restricted Project
hokein accepted D58029: [clangd] Make system header mappings available for PreambleParsedCallback.
Mon, Feb 11, 2:29 AM · Restricted Project, Restricted Project

Fri, Feb 8

hokein accepted D57950: [clangd] Index parameters in function decls.
Fri, Feb 8, 6:30 AM · Restricted Project
hokein accepted D57949: [clang][Index] Add a knob to index function parameters in declarations.

The change looks reasonable, I'd wait for a few days before commit in case anyone has concerns.

Fri, Feb 8, 6:30 AM · Restricted Project, Restricted Project
hokein accepted D57944: [clangd] Fix an assertion in TypoCorrection..
Fri, Feb 8, 5:11 AM · Restricted Project, Restricted Project
hokein added a comment to D57943: [clangd] **Prototype**: clang-tidy-based tweaks.

This is my experiment of playing around tweaks, I think we'll need this.

Fri, Feb 8, 4:03 AM · Restricted Project
hokein added reviewers for D57943: [clangd] **Prototype**: clang-tidy-based tweaks: sammccall, ilya-biryukov.
Fri, Feb 8, 4:03 AM · Restricted Project
hokein updated the diff for D57943: [clangd] **Prototype**: clang-tidy-based tweaks.

Update

Fri, Feb 8, 3:56 AM · Restricted Project
hokein created D57943: [clangd] **Prototype**: clang-tidy-based tweaks.
Fri, Feb 8, 3:56 AM · Restricted Project

Thu, Feb 7

hokein committed rGddd64a4f50bd: [clangd] Fix an assertion failure in Selection. (authored by hokein).
[clangd] Fix an assertion failure in Selection.
Thu, Feb 7, 8:01 AM
hokein committed rCTE353421: [clangd] Fix an assertion failure in Selection..
[clangd] Fix an assertion failure in Selection.
Thu, Feb 7, 8:00 AM
hokein committed rL353421: [clangd] Fix an assertion failure in Selection..
[clangd] Fix an assertion failure in Selection.
Thu, Feb 7, 8:00 AM
hokein closed D57879: [clangd] Fix an assertion failure in Selection..
Thu, Feb 7, 8:00 AM · Restricted Project, Restricted Project
hokein updated the diff for D57879: [clangd] Fix an assertion failure in Selection..

Remove an unexpected change.

Thu, Feb 7, 7:56 AM · Restricted Project, Restricted Project
hokein updated the diff for D57879: [clangd] Fix an assertion failure in Selection..

Add reproduce testcase.

Thu, Feb 7, 7:56 AM · Restricted Project, Restricted Project
hokein updated the diff for D57884: [clangd] Don't restart clangd in vscode extension..

update

Thu, Feb 7, 3:52 AM · Restricted Project
hokein updated the diff for D57884: [clangd] Don't restart clangd in vscode extension..

Fix space.

Thu, Feb 7, 3:51 AM · Restricted Project
hokein created D57884: [clangd] Don't restart clangd in vscode extension..
Thu, Feb 7, 3:48 AM · Restricted Project
hokein added a comment to D56610: [clangd] A code action to qualify an unqualified name.

You'd need to rebase this patch, D57739 had some changes to the Tweak API.

Thu, Feb 7, 1:43 AM · Restricted Project
hokein closed D57739: [clangd] Format tweak's replacements..

Committed in rL353306.

Thu, Feb 7, 1:42 AM · Restricted Project
hokein created D57879: [clangd] Fix an assertion failure in Selection..
Thu, Feb 7, 1:34 AM · Restricted Project, Restricted Project

Wed, Feb 6

hokein committed rGea27b59a8690: [clangd] Bump vscode-clangd v0.0.11 (authored by hokein).
[clangd] Bump vscode-clangd v0.0.11
Wed, Feb 6, 7:30 AM
hokein committed rL353309: [clangd] Bump vscode-clangd v0.0.11.
[clangd] Bump vscode-clangd v0.0.11
Wed, Feb 6, 7:30 AM
hokein committed rCTE353309: [clangd] Bump vscode-clangd v0.0.11.
[clangd] Bump vscode-clangd v0.0.11
Wed, Feb 6, 7:29 AM
hokein committed rG12e194cbb799: [clangd] Format tweak's replacements. (authored by hokein).
[clangd] Format tweak's replacements.
Wed, Feb 6, 7:25 AM
hokein committed rL353306: [clangd] Format tweak's replacements..
[clangd] Format tweak's replacements.
Wed, Feb 6, 7:24 AM
hokein committed rCTE353306: [clangd] Format tweak's replacements..
[clangd] Format tweak's replacements.
Wed, Feb 6, 7:24 AM
hokein updated the diff for D57739: [clangd] Format tweak's replacements..

Remove accident change.

Wed, Feb 6, 7:20 AM · Restricted Project
hokein updated the diff for D57739: [clangd] Format tweak's replacements..

Address comments.

Wed, Feb 6, 7:20 AM · Restricted Project
hokein added inline comments to D57739: [clangd] Format tweak's replacements..
Wed, Feb 6, 5:37 AM · Restricted Project
hokein updated the diff for D57739: [clangd] Format tweak's replacements..

Add missing file.

Wed, Feb 6, 5:36 AM · Restricted Project
hokein updated the diff for D57739: [clangd] Format tweak's replacements..

Address review comments.

Wed, Feb 6, 5:36 AM · Restricted Project
hokein accepted 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:03 AM · Restricted Project, Restricted Project
hokein accepted D57813: [clangd] Enable clangd on Objective-C in VSCode.

Thanks!

Wed, Feb 6, 4:57 AM · Restricted Project
hokein added inline comments to D57739: [clangd] Format tweak's replacements..
Wed, Feb 6, 4:56 AM · Restricted Project
hokein updated the diff for D57739: [clangd] Format tweak's replacements..

Move format to the tweak.

Wed, Feb 6, 4:56 AM · Restricted Project
hokein committed rGac6d2e1b1651: [clangd] Add CLI flag "-clang-tidy" to enable/disable running clang-tidy checks. (authored by hokein).
[clangd] Add CLI flag "-clang-tidy" to enable/disable running clang-tidy checks.
Wed, Feb 6, 1:11 AM
hokein committed rCTE353284: [clangd] Add CLI flag "-clang-tidy" to enable/disable running clang-tidy checks..
[clangd] Add CLI flag "-clang-tidy" to enable/disable running clang-tidy checks.
Wed, Feb 6, 1:11 AM
hokein committed rCTE353283: [clangd] Some minor fixes..
[clangd] Some minor fixes.
Wed, Feb 6, 1:11 AM
hokein committed rL353284: [clangd] Add CLI flag "-clang-tidy" to enable/disable running clang-tidy checks..
[clangd] Add CLI flag "-clang-tidy" to enable/disable running clang-tidy checks.
Wed, Feb 6, 1:10 AM
hokein closed D57746: [clangd] Add CLI flag "-clang-tidy" to enable/disable running clang-tidy checks..
Wed, Feb 6, 1:10 AM · Restricted Project, Restricted Project
hokein updated the diff for D57746: [clangd] Add CLI flag "-clang-tidy" to enable/disable running clang-tidy checks..

Set the flag to false by default.

Wed, Feb 6, 1:10 AM · Restricted Project, Restricted Project
hokein committed rG5dcc66d0b8dd: [clangd] Some minor fixes. (authored by hokein).
[clangd] Some minor fixes.
Wed, Feb 6, 1:09 AM
hokein committed rL353283: [clangd] Some minor fixes..
[clangd] Some minor fixes.
Wed, Feb 6, 1:08 AM
hokein closed D57755: [clangd] Some minor fixes..
Wed, Feb 6, 1:08 AM · Restricted Project, Restricted Project

Tue, Feb 5

hokein created D57755: [clangd] Some minor fixes..
Tue, Feb 5, 7:35 AM · Restricted Project, Restricted Project
hokein 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);
   }

protected:
   virtual Replacements doApply() = 0; // inheritors should implement this now. Note: the name is terrible, suggestions are welcome.
};

This would ensure the callers don't need to deal with the formatting on their own.

Tue, Feb 5, 7:19 AM · Restricted Project
hokein updated the diff for D57739: [clangd] Format tweak's replacements..

Adress review comments.

Tue, Feb 5, 7:18 AM · Restricted Project
hokein created D57746: [clangd] Add CLI flag "-clang-tidy" to enable/disable running clang-tidy checks..
Tue, Feb 5, 4:40 AM · Restricted Project, Restricted Project
hokein added inline comments to D57739: [clangd] Format tweak's replacements..
Tue, Feb 5, 2:34 AM · Restricted Project
hokein created D57739: [clangd] Format tweak's replacements..
Tue, Feb 5, 2:27 AM · Restricted Project

Mon, Feb 4

hokein accepted D57353: [clang-tidy] Add the abseil-duration-unnecessary-conversion check.

LGTM

Mon, Feb 4, 10:39 AM · Restricted Project, Restricted Project
hokein added a comment to D57353: [clang-tidy] Add the abseil-duration-unnecessary-conversion check.

The code looks good, but I have a concern about the check name -- double seems a confusing word, see my comment.

Mon, Feb 4, 3:08 AM · Restricted Project, Restricted Project
hokein committed rG8e933c1ccf1d: [clangd] Bump vscode-clangd v0.0.10 (authored by hokein).
[clangd] Bump vscode-clangd v0.0.10
Mon, Feb 4, 1:27 AM
hokein committed rCTE353027: [clangd] Bump vscode-clangd v0.0.10.
[clangd] Bump vscode-clangd v0.0.10
Mon, Feb 4, 1:27 AM
hokein committed rL353027: [clangd] Bump vscode-clangd v0.0.10.
[clangd] Bump vscode-clangd v0.0.10
Mon, Feb 4, 1:26 AM
hokein committed rGa1b059691699: [clangd] Update vscode dependencies (authored by hokein).
[clangd] Update vscode dependencies
Mon, Feb 4, 1:21 AM
hokein committed rCTE353026: [clangd] Update vscode dependencies.
[clangd] Update vscode dependencies
Mon, Feb 4, 1:21 AM
hokein committed rL353026: [clangd] Update vscode dependencies.
[clangd] Update vscode dependencies
Mon, Feb 4, 1:21 AM

Fri, Feb 1

hokein accepted D57580: [Clangd] textDocument/definition and textDocument/declaration "bounce" between definition and declaration location when they are distinct..

LGTM

Fri, Feb 1, 8:22 AM · Restricted Project, Restricted Project
hokein accepted D57573: Disable tidy checks with too many hits.

I'm +1 on disabling these checks as they add too much noise in editors (even though the naming check is correct, we can't do a global cleanup on the codebase). Let's commit it, we could revert it if someone has other concerns.

Fri, Feb 1, 2:57 AM · Restricted Project, Restricted Project
hokein accepted D57388: [clangd] Implement textDocument/declaration from LSP 3.14.
Fri, Feb 1, 12:12 AM · Restricted Project

Wed, Jan 30

hokein added inline comments to D57325: [clangd] Collect macros in static index..
Wed, Jan 30, 3:37 AM · Restricted Project
hokein added a comment to D57388: [clangd] Implement textDocument/declaration from LSP 3.14.

Looks good overall, most are nits.

Wed, Jan 30, 3:03 AM · Restricted Project

Tue, Jan 29

hokein accepted D57384: [clangd] Make -clang-tidy-checks a non-hidden command-line arg.
Tue, Jan 29, 7:20 AM
hokein added a comment to D57057: [clangd] Log clang-tidy configuration, NFC.

This turns out to be excessively spammy in unit tests, can we change this to dlog()?

Tue, Jan 29, 4:32 AM
hokein committed rL352485: [clangd] dlog clang-tidy configuration.
[clangd] dlog clang-tidy configuration
Tue, Jan 29, 4:32 AM
hokein committed rCTE352485: [clangd] dlog clang-tidy configuration.
[clangd] dlog clang-tidy configuration
Tue, Jan 29, 4:32 AM

Mon, Jan 28

hokein created D57325: [clangd] Collect macros in static index..
Mon, Jan 28, 6:51 AM · Restricted Project
hokein committed rCTE352367: [clangd] Index main-file macros (bug 39761).
[clangd] Index main-file macros (bug 39761)
Mon, Jan 28, 6:12 AM
hokein committed rL352367: [clangd] Index main-file macros (bug 39761).
[clangd] Index main-file macros (bug 39761)
Mon, Jan 28, 6:12 AM
hokein closed D55739: [Clangd] Index main-file macros (bug 39761).
Mon, Jan 28, 6:12 AM
hokein committed rCTE352364: [clang-tidy] Fix a build error..
[clang-tidy] Fix a build error.
Mon, Jan 28, 6:08 AM
hokein committed rL352364: [clang-tidy] Fix a build error..
[clang-tidy] Fix a build error.
Mon, Jan 28, 6:08 AM
hokein added a comment to D55739: [Clangd] Index main-file macros (bug 39761).

Measured this patch on LLVM, the increasing number of symbols is reasonable (from 422 K to 425 K).

Mon, Jan 28, 6:03 AM
hokein accepted D55739: [Clangd] Index main-file macros (bug 39761).
Mon, Jan 28, 3:05 AM

Fri, Jan 25

hokein committed rL352205: [clangd] NFC: fix clang-tidy warnings..
[clangd] NFC: fix clang-tidy warnings.
Fri, Jan 25, 7:15 AM
hokein committed rCTE352205: [clangd] NFC: fix clang-tidy warnings..
[clangd] NFC: fix clang-tidy warnings.
Fri, Jan 25, 7:15 AM
hokein committed rL352197: gitignore: ignore clangd index files..
gitignore: ignore clangd index files.
Fri, Jan 25, 6:05 AM
hokein closed D57227: gitignore: ignore clangd index files..
Fri, Jan 25, 6:05 AM
hokein created D57227: gitignore: ignore clangd index files..
Fri, Jan 25, 3:49 AM