ioeric (Eric Liu)Administrator
User

Projects

User does not belong to any projects.

User Details

User Since
Feb 12 2016, 3:46 AM (136 w, 7 h)
Roles
Administrator

Recent Activity

Today

ioeric created D52364: [clangd] Initial supoprt for cross-namespace global code completion..
Fri, Sep 21, 9:27 AM
ioeric added inline comments to D52047: [clangd] Add building benchmark and memory consumption tracking.
Fri, Sep 21, 8:12 AM · Restricted Project
ioeric accepted D52357: [clangd] Force Dex to respect symbol collector flags.
Fri, Sep 21, 7:34 AM · Restricted Project
ioeric added inline comments to D52357: [clangd] Force Dex to respect symbol collector flags.
Fri, Sep 21, 6:36 AM · Restricted Project
ioeric committed rCTE342730: [clangd] Remember to serialize symbol origin in YAML..
[clangd] Remember to serialize symbol origin in YAML.
Fri, Sep 21, 6:09 AM
ioeric committed rL342730: [clangd] Remember to serialize symbol origin in YAML..
[clangd] Remember to serialize symbol origin in YAML.
Fri, Sep 21, 6:07 AM
ioeric added a comment to D52300: [clangd] Implement VByte PostingList compression.

Also, I'll refine D52047 a little bit and I believe that is should be way easier to understand performance + memory consumption once we have these benchmarks in. Both @ioeric and @ilya-biryukov expressed their concern with regard to the memory consumption "benchmark" and suggested a separate binary. While this seems fine to me, I think it's important to keep performance + memory tracking infrastructure easy to use (in this sense scattering different metrics across multiple binaries makes it less accessible and probably introduce some code duplication) and therefore using this "trick" is OK to me, but I don't have a strong opinion about this. What do you think, @sammccall?

FWIW, I think the "trick" for memory benchmark is fine. I just think we should add proper output to make the trick clear to users, as suggested in the patch comment.

Fri, Sep 21, 5:16 AM · Restricted Project
ioeric added a comment to D52225: [clang] Implement Override Suggestions in Sema..

Could you move the corresponding tests from clangd to sema?

Fri, Sep 21, 2:13 AM
ioeric added a comment to D52273: [clangd] Initial implementation of expected types.

This seems very clever, but extremely complicated - you've implemented much of C++'s conversion logic, it's not clear to me which parts are actually necessary to completion quality.
(Honestly this applies to expected types overall - it seems intuitively likely that it's a good signal, it seems less obvious that it pulls its weight if it can't be made simple).

From the outside it seems much of it is YAGNI, and if we do then we need to build it up slowly with an eye for maintainability.
Can we start with expected type boosting (no conversions) as previously discussed, and later measure which other parts make a difference? (I think we'll need/want the simple model anyway, for this to work with Dex and other token-based indexes).

Fri, Sep 21, 2:00 AM

Wed, Sep 19

ioeric created D52264: Deduplicate replacements from diagnostics..
Wed, Sep 19, 6:12 AM
ioeric committed rL342529: [clangd] Store preamble macros in dynamic index..
[clangd] Store preamble macros in dynamic index.
Wed, Sep 19, 2:36 AM
ioeric committed rCTE342529: [clangd] Store preamble macros in dynamic index..
[clangd] Store preamble macros in dynamic index.
Wed, Sep 19, 2:36 AM
ioeric closed D52078: [clangd] Store preamble macros in dynamic index..
Wed, Sep 19, 2:36 AM
ioeric committed rL342528: [Sema] Do not load macros from preamble when LoadExternal is false..
[Sema] Do not load macros from preamble when LoadExternal is false.
Wed, Sep 19, 2:36 AM
ioeric committed rC342528: [Sema] Do not load macros from preamble when LoadExternal is false..
[Sema] Do not load macros from preamble when LoadExternal is false.
Wed, Sep 19, 2:36 AM
ioeric closed D52079: [Sema] Do not load macros from preamble when LoadExternal is false..
Wed, Sep 19, 2:36 AM
ioeric added inline comments to D52233: [dexp] Dump JSON representations of fuzzy find requests.
Wed, Sep 19, 1:52 AM · Restricted Project
ioeric accepted D52250: [clangd] expose MergedIndex class.

lgtm

Wed, Sep 19, 1:49 AM
ioeric added inline comments to D52047: [clangd] Add building benchmark and memory consumption tracking.
Wed, Sep 19, 1:42 AM · Restricted Project

Tue, Sep 18

ioeric added a comment to D52078: [clangd] Store preamble macros in dynamic index..

Something not listed in cons: because macros aren't namespaced and we don't have lots of signals, they can be really spammy.
Potentially, offering macros that aren't in the TU could be a loss even if it's a win for other types of signals.

Aren't they already spammy from Sema? Sema can provide thousands of macros in the TU.

Tue, Sep 18, 7:28 AM
ioeric updated the diff for D52079: [Sema] Do not load macros from preamble when LoadExternal is false..
  • added lit test
Tue, Sep 18, 7:15 AM
ioeric accepted D52084: [clangd] Improve PostingList iterator string representation.
Tue, Sep 18, 7:15 AM · Restricted Project
ioeric updated the diff for D52078: [clangd] Store preamble macros in dynamic index..
  • merge with origin/master
Tue, Sep 18, 6:49 AM
ioeric added inline comments to D52222: [clangd] Merge ClangdServer::DynamicIndex into FileIndex. NFC..
Tue, Sep 18, 6:37 AM
ioeric committed rL342473: [clangd] Get rid of Decls parameter in indexMainDecls. NFC.
[clangd] Get rid of Decls parameter in indexMainDecls. NFC
Tue, Sep 18, 6:36 AM
ioeric committed rCTE342473: [clangd] Get rid of Decls parameter in indexMainDecls. NFC.
[clangd] Get rid of Decls parameter in indexMainDecls. NFC
Tue, Sep 18, 6:36 AM
ioeric committed rL342460: [clangd] Merge ClangdServer::DynamicIndex into FileIndex. NFC..
[clangd] Merge ClangdServer::DynamicIndex into FileIndex. NFC.
Tue, Sep 18, 3:32 AM
ioeric committed rCTE342460: [clangd] Merge ClangdServer::DynamicIndex into FileIndex. NFC..
[clangd] Merge ClangdServer::DynamicIndex into FileIndex. NFC.
Tue, Sep 18, 3:32 AM
ioeric closed D52222: [clangd] Merge ClangdServer::DynamicIndex into FileIndex. NFC..
Tue, Sep 18, 3:32 AM
ioeric closed D52222: [clangd] Merge ClangdServer::DynamicIndex into FileIndex. NFC..
Tue, Sep 18, 3:32 AM
ioeric updated the diff for D52222: [clangd] Merge ClangdServer::DynamicIndex into FileIndex. NFC..
  • addressed review comments
Tue, Sep 18, 3:25 AM
ioeric added inline comments to D52222: [clangd] Merge ClangdServer::DynamicIndex into FileIndex. NFC..
Tue, Sep 18, 3:25 AM
ioeric committed rCTE342452: [clangd] Adapt API change after 342451..
[clangd] Adapt API change after 342451.
Tue, Sep 18, 1:56 AM
ioeric committed rL342452: [clangd] Adapt API change after 342451..
[clangd] Adapt API change after 342451.
Tue, Sep 18, 1:56 AM
ioeric committed rC342451: [Index] Add an option to collect macros from preprocesor..
[Index] Add an option to collect macros from preprocesor.
Tue, Sep 18, 1:52 AM
ioeric committed rL342451: [Index] Add an option to collect macros from preprocesor..
[Index] Add an option to collect macros from preprocesor.
Tue, Sep 18, 1:52 AM
ioeric closed D52098: [Index] Add an option to collect macros from preprocesor..
Tue, Sep 18, 1:52 AM
ioeric added inline comments to D52098: [Index] Add an option to collect macros from preprocesor..
Tue, Sep 18, 1:13 AM
ioeric created D52222: [clangd] Merge ClangdServer::DynamicIndex into FileIndex. NFC..
Tue, Sep 18, 1:12 AM

Mon, Sep 17

ioeric added inline comments to D52098: [Index] Add an option to collect macros from preprocesor..
Mon, Sep 17, 7:05 AM
ioeric updated the diff for D52098: [Index] Add an option to collect macros from preprocesor..
  • addressed review comments.
Mon, Sep 17, 7:05 AM
ioeric added a comment to D52047: [clangd] Add building benchmark and memory consumption tracking.

The benchmark change looks fine to me. But I'm not very familiar with the trick, so I'll let Sam (who proposed the idea as you mentioned), stamp the patch.

Mon, Sep 17, 12:53 AM · Restricted Project
ioeric committed rCTE342362: [clangd] Get rid of AST matchers in SymbolCollector. NFC.
[clangd] Get rid of AST matchers in SymbolCollector. NFC
Mon, Sep 17, 12:50 AM
ioeric committed rL342362: [clangd] Get rid of AST matchers in SymbolCollector. NFC.
[clangd] Get rid of AST matchers in SymbolCollector. NFC
Mon, Sep 17, 12:45 AM
ioeric closed D52089: [clangd] Get rid of AST matchers in SymbolCollector. NFC.
Mon, Sep 17, 12:45 AM
ioeric accepted D52084: [clangd] Improve PostingList iterator string representation.
Mon, Sep 17, 12:44 AM · Restricted Project

Fri, Sep 14

ioeric added a comment to D52089: [clangd] Get rid of AST matchers in SymbolCollector. NFC.

why?

Fri, Sep 14, 11:55 PM
ioeric updated the summary of D52089: [clangd] Get rid of AST matchers in SymbolCollector. NFC.
Fri, Sep 14, 11:51 PM
ioeric added inline comments to D52078: [clangd] Store preamble macros in dynamic index..
Fri, Sep 14, 11:36 PM
ioeric added a comment to D52078: [clangd] Store preamble macros in dynamic index..

~1% increase in memory usage seems totally fine. Actually surprised it's that small.

Tested on a larger file: it's ~5% for ClangdServer.cpp. I think it's still worth it for the speedup.

Fri, Sep 14, 8:28 AM
ioeric updated the diff for D52078: [clangd] Store preamble macros in dynamic index..
  • Move macro collection to indexTopLevelDecls.
Fri, Sep 14, 8:26 AM
ioeric added a dependency for D52078: [clangd] Store preamble macros in dynamic index.: D52098: [Index] Add an option to collect macros from preprocesor..
Fri, Sep 14, 8:21 AM
ioeric added a dependent revision for D52098: [Index] Add an option to collect macros from preprocesor.: D52078: [clangd] Store preamble macros in dynamic index..
Fri, Sep 14, 8:21 AM
ioeric updated the diff for D52098: [Index] Add an option to collect macros from preprocesor..
  • another cleanup...
Fri, Sep 14, 8:15 AM
ioeric updated the diff for D52098: [Index] Add an option to collect macros from preprocesor..
  • remove debug message.
Fri, Sep 14, 8:08 AM
ioeric created D52098: [Index] Add an option to collect macros from preprocesor..
Fri, Sep 14, 8:07 AM
ioeric created D52089: [clangd] Get rid of AST matchers in SymbolCollector. NFC.
Fri, Sep 14, 5:54 AM
ioeric added inline comments to D52078: [clangd] Store preamble macros in dynamic index..
Fri, Sep 14, 4:38 AM
ioeric updated the summary of D52078: [clangd] Store preamble macros in dynamic index..
Fri, Sep 14, 1:33 AM
ioeric updated the diff for D52078: [clangd] Store preamble macros in dynamic index..
  • minor cleanup
Fri, Sep 14, 1:29 AM
ioeric added a dependency for D52078: [clangd] Store preamble macros in dynamic index.: D52079: [Sema] Do not load macros from preamble when LoadExternal is false..
Fri, Sep 14, 1:23 AM
ioeric added a dependent revision for D52079: [Sema] Do not load macros from preamble when LoadExternal is false.: D52078: [clangd] Store preamble macros in dynamic index..
Fri, Sep 14, 1:23 AM
ioeric created D52079: [Sema] Do not load macros from preamble when LoadExternal is false..
Fri, Sep 14, 1:20 AM
ioeric created D52078: [clangd] Store preamble macros in dynamic index..
Fri, Sep 14, 1:19 AM

Thu, Sep 13

ioeric committed rCTE342134: [clangd] Clarify and hide -index flag..
[clangd] Clarify and hide -index flag.
Thu, Sep 13, 5:54 AM
ioeric committed rL342134: [clangd] Clarify and hide -index flag..
[clangd] Clarify and hide -index flag.
Thu, Sep 13, 5:54 AM
ioeric closed D51977: [clangd] Clarify and hide -index flag..
Thu, Sep 13, 5:54 AM
ioeric updated the diff for D51977: [clangd] Clarify and hide -index flag..
  • Remove static.
Thu, Sep 13, 5:54 AM
ioeric added inline comments to D51982: [clangd] Introduce PostingList interface.
Thu, Sep 13, 4:39 AM · Restricted Project
ioeric accepted D51297: [docs] Provide pointers to known editor plugins and extensions.
Thu, Sep 13, 1:05 AM · Restricted Project
ioeric added inline comments to D51297: [docs] Provide pointers to known editor plugins and extensions.
Thu, Sep 13, 1:05 AM · Restricted Project
ioeric added inline comments to D51297: [docs] Provide pointers to known editor plugins and extensions.
Thu, Sep 13, 12:29 AM · Restricted Project

Wed, Sep 12

ioeric accepted D51987: [clangd] Rename global-symbol-builder to clangd-indexer..

You beat it to me, but I thought we didn't use indexer as it could be confused with the actual indexing?

Sorry. Not a big deal, can revert back if needed.
Mostly like indexer since it's shorter. Also not very confusing, since we call the thing it produces an "index", so calling it an "indexer" seems fine.

What's the index you're referring to? Building data structures for faster search?

Yes. The current symbol builder produces *symbol table*, which can be easily confused with an index. We might have a different tool that builds the symbol index structure (e.g. serialized dex), which might be a better candidate for the name "indexer".

Part of the reason I like "indexer" here is I don't think it should be a separate tool - it'd be nice if this tool would spit out a RIFF file with Dex posting lists ready to be loaded.
(For huge codebases, things look different - but we've been talking about using a different entrypoint for those anyway due to bifurcation around merge-on-the-fly etc)

I wasn't aware that we are also expanding the scope of the symbol builder. If being short is not the only reason here, I have no problem.

Wed, Sep 12, 8:41 AM
ioeric added a comment to D51987: [clangd] Rename global-symbol-builder to clangd-indexer..

You beat it to me, but I thought we didn't use indexer as it could be confused with the actual indexing?

Sorry. Not a big deal, can revert back if needed.
Mostly like indexer since it's shorter. Also not very confusing, since we call the thing it produces an "index", so calling it an "indexer" seems fine.

What's the index you're referring to? Building data structures for faster search?

Yes. The current symbol builder produces *symbol table*, which can be easily confused with an index. We might have a different tool that builds the symbol index structure (e.g. serialized dex), which might be a better candidate for the name "indexer".

Wed, Sep 12, 8:11 AM
ioeric added a comment to D51987: [clangd] Rename global-symbol-builder to clangd-indexer..

clangd-indexer looks nice and short. @ioeric, WDYT?

Wed, Sep 12, 8:01 AM
ioeric updated subscribers of D51987: [clangd] Rename global-symbol-builder to clangd-indexer..

I mean clangd-symbol-builder

Wed, Sep 12, 7:33 AM
ioeric accepted D51987: [clangd] Rename global-symbol-builder to clangd-indexer..

+1 to clang-symbol-builder

Wed, Sep 12, 7:32 AM
ioeric created D51977: [clangd] Clarify and hide -index flag..
Wed, Sep 12, 4:56 AM
ioeric committed rC342028: [Tooling] Wait for all threads to finish before resetting CWD..
[Tooling] Wait for all threads to finish before resetting CWD.
Wed, Sep 12, 1:31 AM
ioeric committed rL342028: [Tooling] Wait for all threads to finish before resetting CWD..
[Tooling] Wait for all threads to finish before resetting CWD.
Wed, Sep 12, 1:31 AM

Tue, Sep 11

ioeric added inline comments to D51860: [clangd] NFC: Use uint32_t for FuzzyFindRequest limits.
Tue, Sep 11, 2:41 AM · Restricted Project
ioeric accepted D51860: [clangd] NFC: Use uint32_t for FuzzyFindRequest limits.
Tue, Sep 11, 1:32 AM · Restricted Project

Mon, Sep 10

ioeric accepted D51864: [Tooling] Restore working dir in ClangTool..

lg

Mon, Sep 10, 10:08 AM
ioeric added inline comments to D51860: [clangd] NFC: Use uint32_t for FuzzyFindRequest limits.
Mon, Sep 10, 8:56 AM · Restricted Project
ioeric added a comment to D51297: [docs] Provide pointers to known editor plugins and extensions.

I would stamp this from my side, but concerns whether we should recommend YCM's LSP-based completer instead are probably still there.
@sammccall, WDYT?

Yes, I can see your point, but I think this is better than nothing (which we currently have). Also, having the guide in LanguageClient-neovim Wiki might be easier for the users since they can change something (e.g. when the plugin is updated and the docs become outdated) and have an easier time finding out about the option.

Would love to hear some feedback from @ioeric and @sammccall.

I also support having some instructions/pointers on editor integration. That said, I think we should have a section "Editor integration" with a list of editor clients that are known to work with clangd, instead of having a section just for vim. Something like:

#Editor (or client?) integration
Mon, Sep 10, 3:48 AM · Restricted Project
ioeric accepted D51539: [clangd] Add symbol slab size to index memory consumption estimates.
Mon, Sep 10, 3:37 AM · Restricted Project

Fri, Sep 7

ioeric added a comment to D51747: [clangd] Show deprecation diagnostics.

Not sure if it's fine to hijack our own diagnostic-specific flags in to clang command args.

Cons that I see:

  1. There is no way for the users to turn them off if they find them non-useful. If we add a way, it would be more config parameters which overlap with other mechanism that we have - compiler flags.
  2. Users who are used to having them as warnings will now see them as notes. Again, no way to tweak this behavior.

    What's our use-case? Maybe we should ask the clients to add -Wdeprecated if they care about those?

    PS In case I'm missing the context here, please let me know.

Instead of

Fri, Sep 7, 2:17 PM
ioeric committed rL341645: [clangd] Canonicalize include paths in clangd..
[clangd] Canonicalize include paths in clangd.
Fri, Sep 7, 2:41 AM
ioeric committed rCTE341645: [clangd] Canonicalize include paths in clangd..
[clangd] Canonicalize include paths in clangd.
Fri, Sep 7, 2:41 AM
ioeric added inline comments to D51539: [clangd] Add symbol slab size to index memory consumption estimates.
Fri, Sep 7, 1:57 AM · Restricted Project
ioeric added inline comments to D51747: [clangd] Show deprecation diagnostics.
Fri, Sep 7, 1:51 AM
ioeric added inline comments to D51539: [clangd] Add symbol slab size to index memory consumption estimates.
Fri, Sep 7, 1:33 AM · Restricted Project

Thu, Sep 6

ioeric committed rL341576: [clangd] Add "Deprecated" field to Symbol and CodeCompletion..
[clangd] Add "Deprecated" field to Symbol and CodeCompletion.
Thu, Sep 6, 11:53 AM
ioeric committed rCTE341576: [clangd] Add "Deprecated" field to Symbol and CodeCompletion..
[clangd] Add "Deprecated" field to Symbol and CodeCompletion.
Thu, Sep 6, 11:53 AM
ioeric closed D51724: [clangd] Add "Deprecated" field to Symbol and CodeCompletion..
Thu, Sep 6, 11:53 AM
ioeric updated the diff for D51724: [clangd] Add "Deprecated" field to Symbol and CodeCompletion..
  • merged with origin/master
Thu, Sep 6, 11:52 AM
ioeric updated the diff for D51724: [clangd] Add "Deprecated" field to Symbol and CodeCompletion..
  • addressed review comments
Thu, Sep 6, 11:51 AM
ioeric added inline comments to D51724: [clangd] Add "Deprecated" field to Symbol and CodeCompletion..
Thu, Sep 6, 6:47 AM
ioeric updated the diff for D51724: [clangd] Add "Deprecated" field to Symbol and CodeCompletion..
  • Pack flags in Symbol.
Thu, Sep 6, 6:47 AM
ioeric committed rCTE341534: [clangd] Set SymbolID for sema macros so that they can be merged with index….
[clangd] Set SymbolID for sema macros so that they can be merged with index…
Thu, Sep 6, 3:01 AM