ioeric (Eric Liu)Administrator
User

Projects

User does not belong to any projects.

User Details

User Since
Feb 12 2016, 3:46 AM (126 w, 3 d)
Roles
Administrator

Recent Activity

Today

ioeric added a comment to D48395: Added PublicOnly flag.

Remember to mark comments as done when they are. Otherwise, LGTM unless @ioeric has any concerns.

No concern if this looks good to Julie.

Mon, Jul 16, 1:20 AM

Thu, Jul 12

ioeric committed rC336928: [Tooling] Make standalone executor support user-provided vfs..
[Tooling] Make standalone executor support user-provided vfs.
Thu, Jul 12, 11:37 AM
ioeric committed rL336928: [Tooling] Make standalone executor support user-provided vfs..
[Tooling] Make standalone executor support user-provided vfs.
Thu, Jul 12, 11:37 AM
ioeric committed rL336910: [Tooling] Get working directory properly without assuming real file system..
[Tooling] Get working directory properly without assuming real file system.
Thu, Jul 12, 7:59 AM
ioeric committed rC336910: [Tooling] Get working directory properly without assuming real file system..
[Tooling] Get working directory properly without assuming real file system.
Thu, Jul 12, 7:59 AM
ioeric added a comment to D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name.

For example, suppose you have header search directories -I/path/to/include and -I. in your compile command. When preprocessor searches for an #include "x.h", it will try to stat "/path/to/include/x.h" and "./x.h" and take the first one that exists. This can introduce indeterminism for the path (./x.h or /abs/x.h) you later get for the header file, e.g. when you try to look up file name by FileID through SourceManager. The path you get for a FileEntry or FileID would depend on how clang looks up a file and how a file is first opened into SourceManager/FileManager. It seems that the current behavior of clangd + in-memory file system would give you paths that are relative to the working directory for both cases. I'm not sure if that's the best behavior, but I think the consistency has its value. For example, in unit tests where in-memory file systems are heavily used, it's important to have a way to compare the reported file path (e.g. file path corresponding to a source location) with the expected paths. We could choose to always return real path, which could be potentially expensive, or we could require users to always compare real paths (it's unclear how well this would work though e.g. ClangTool doesn't expose its vfs), but they need to be enforced by the API. Otherwise, I worry it would cause more confusions for folks who use clang with in-memory file system in the future.

It's hard to tell without seeing an actual failing use case.

I think the clang-move test you mitigated in D48951 is an example. When setting up compiler instance, it uses -I. in the compile command (https://github.com/llvm-mirror/clang-tools-extra/blob/master/unittests/clang-move/ClangMoveTests.cpp#L236), which results in the header paths that start with ./. If you change replace . with the absolute path of the working directory e.g. -I/usr/include, the paths you get will start with /usr/include/. This is caused by the way preprocessor looks up include headers. We could require users (e.g. the clang-move test writer) to clean up dots, but this has to be enforced somehow by the framework.

I understand the argument, but I think the necessity to work as closely as RealFileSystem as possible is important. Otherwise, it becomes impossible to reproduce bugs happening in the real world in unittests.

FWIW, I don't think we could reproduce all real world bugs with unit tests ;) But I agree with you that we should try to converge the behavior if possible, and I support the motivation of this change ;) This change would be perfectly fine as is if in-mem fs is always used directly by users, but the problem is that it's also used inside clang and clang tooling, where users don't control over how files are opened. My point is we should do this in a way that doesn't introduce inconsistency/confusion for users of clang and tooling framework.

Thu, Jul 12, 7:12 AM
ioeric added a comment to D49197: FileManager: Try to compute RealPathName in getVirtualFile.

Background: I'm trying to fix the cases why we receive a FileEntry without a real path computed in clangd, so we can avoid having to compute it ourselves.

I'm curious how you use getVirtualFile in your clangd tests. In general, I'd highly recommend virtual file system in favor of remapped files for clangd tests.

Thu, Jul 12, 12:41 AM

Wed, Jul 11

ioeric added a comment to D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name.

After looking at a few more use cases of the in-memory file system, I think a problem we need to address is the consistency of file paths you get from clang when using in-mem vfs. The clang-move tests that you have mitigated in D48951 could be an example.

Wed, Jul 11, 2:50 PM
ioeric committed rC336831: Revert "[VirtualFileSystem] InMemoryFileSystem::status: Return a Status with….
Revert "[VirtualFileSystem] InMemoryFileSystem::status: Return a Status with…
Wed, Jul 11, 11:48 AM
ioeric committed rL336831: Revert "[VirtualFileSystem] InMemoryFileSystem::status: Return a Status with….
Revert "[VirtualFileSystem] InMemoryFileSystem::status: Return a Status with…
Wed, Jul 11, 11:48 AM
ioeric added inline comments to D49197: FileManager: Try to compute RealPathName in getVirtualFile.
Wed, Jul 11, 11:42 AM
ioeric added a comment to D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name.

Would you mind reverting this patch for now so that we can come up with a solution to address those use cases?

Sorry again about missing the discussion earlier!

Of course, feel free to revert if needed (I'm not sure how to do that). Are you able to come up with a test case that covers the use case you mention?

Wed, Jul 11, 11:34 AM
ioeric added a reviewer for D49197: FileManager: Try to compute RealPathName in getVirtualFile: ioeric.
Wed, Jul 11, 11:27 AM
ioeric added a comment to D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name.

With the InMemoryFileSystem, this now returns a non-real path. The result is that we fill RealPathName with that non-real path. I see two options here:

  1. Either the FileManager is wrong to assume that File::getName returns a real path, and should call FS->getRealPath to do the job.
  2. If the contract is that the File::getName interface should return a real path, then we should fix the File::getName implementation to do that somehow.

    I would opt for 1. This way, we could compute the RealPathName field even if we don't open the file (unlike currently).

I'd also say FileManager should use FileSystem::getRealPath. The code that does it differently was there before FileSystem::getRealPath was added.
And RealFile should probably not do any magic in getName, we could add a separate method for (getRealName?) if that's absolutely needed.

Refactorings in that area would probably break stuff and won't be trivial and I don't think this change should be blocked by those. So I'd be happy if this landed right away with a FIXME in FileManager mentioning that InMemoryFileSystem might give surprising results there.
@ioeric added FileSystem::getRealPath, he may more ideas on how we should proceed.

Sorry for being late in the discussion. I'm not sure if getRealPath is the right thing to do in FileManager as it also supports virtual files added via FileManager::getVirtualFile, and they don't necessary have a real path. This would also break users of ClangTool::mapVirtualFile when the mapped files are relative. As a matter of fact, I'm already seeing related breakages in our downstream branch :(

Would you mind reverting this patch for now so that we can come up with a solution to address those use cases?

Sorry again about missing the discussion earlier!

Wed, Jul 11, 11:08 AM
ioeric added a comment to D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name.

With the InMemoryFileSystem, this now returns a non-real path. The result is that we fill RealPathName with that non-real path. I see two options here:

  1. Either the FileManager is wrong to assume that File::getName returns a real path, and should call FS->getRealPath to do the job.
  2. If the contract is that the File::getName interface should return a real path, then we should fix the File::getName implementation to do that somehow.

    I would opt for 1. This way, we could compute the RealPathName field even if we don't open the file (unlike currently).

I'd also say FileManager should use FileSystem::getRealPath. The code that does it differently was there before FileSystem::getRealPath was added.
And RealFile should probably not do any magic in getName, we could add a separate method for (getRealName?) if that's absolutely needed.

Refactorings in that area would probably break stuff and won't be trivial and I don't think this change should be blocked by those. So I'd be happy if this landed right away with a FIXME in FileManager mentioning that InMemoryFileSystem might give surprising results there.
@ioeric added FileSystem::getRealPath, he may more ideas on how we should proceed.

Sorry for being late in the discussion. I'm not sure if getRealPath is the right thing to do in FileManager as it also supports virtual files added via FileManager::getVirtualFile, and they don't necessary have a real path. This would also break users of ClangTool::mapVirtualFile when the mapped files are relative. As a matter of fact, I'm already seeing related breakages in our downstream branch :(

Wed, Jul 11, 11:00 AM
ioeric committed rCTE336801: [clangd] Ignore sema code complete callback with recovery context..
[clangd] Ignore sema code complete callback with recovery context.
Wed, Jul 11, 6:20 AM
ioeric committed rL336801: [clangd] Ignore sema code complete callback with recovery context..
[clangd] Ignore sema code complete callback with recovery context.
Wed, Jul 11, 6:20 AM
ioeric closed D49175: [clangd] Ignore sema code complete callback with recovery context..
Wed, Jul 11, 6:20 AM
ioeric added a comment to D49175: [clangd] Ignore sema code complete callback with recovery context..

I like this idea, of course hard to know how it will affect all practical cases.

Is it easy to get have internal stats on how many wins/losses this has?

This would fix bad completion results when sema could recover to the normal mode (e.g. completion in macros like EXPECT_THAT, expressions in if statements). From the code completion metrics I collected from existing code, the number of completions where no result was found dropped by 30%. The loss seems to be we will get no result in recovery mode rather than bad results (e.g. in excluded preprocessor branches).

Wed, Jul 11, 6:18 AM
ioeric updated the diff for D49175: [clangd] Ignore sema code complete callback with recovery context..
  • Addressed review comments.
Wed, Jul 11, 5:54 AM
ioeric created D49175: [clangd] Ignore sema code complete callback with recovery context..
Wed, Jul 11, 5:30 AM
ioeric committed rL336770: [WebAssembly] Only call llvm::value::dump() in debug build..
[WebAssembly] Only call llvm::value::dump() in debug build.
Wed, Jul 11, 1:21 AM

Mon, Jul 9

ioeric committed rC336584: [Index] Ignore noop #undef's when handling macro occurrences..
[Index] Ignore noop #undef's when handling macro occurrences.
Mon, Jul 9, 12:07 PM
ioeric committed rL336584: [Index] Ignore noop #undef's when handling macro occurrences..
[Index] Ignore noop #undef's when handling macro occurrences.
Mon, Jul 9, 12:07 PM
ioeric committed rCTE336581: [clangd] Make sure macro information exists before increasing usage count..
[clangd] Make sure macro information exists before increasing usage count.
Mon, Jul 9, 11:59 AM
ioeric committed rL336581: [clangd] Make sure macro information exists before increasing usage count..
[clangd] Make sure macro information exists before increasing usage count.
Mon, Jul 9, 11:59 AM
ioeric committed rL336553: [clangd] Support indexing MACROs..
[clangd] Support indexing MACROs.
Mon, Jul 9, 8:36 AM
ioeric committed rCTE336553: [clangd] Support indexing MACROs..
[clangd] Support indexing MACROs.
Mon, Jul 9, 8:36 AM
ioeric closed D49028: [clangd] Support indexing MACROs..
Mon, Jul 9, 8:36 AM
ioeric added inline comments to D49028: [clangd] Support indexing MACROs..
Mon, Jul 9, 3:10 AM
ioeric updated the diff for D49028: [clangd] Support indexing MACROs..
  • Addressed review comments.
Mon, Jul 9, 3:09 AM
ioeric committed rL336532: Try to fix build bot after r336524.
Try to fix build bot after r336524
Mon, Jul 9, 2:22 AM
ioeric committed rC336532: Try to fix build bot after r336524.
Try to fix build bot after r336524
Mon, Jul 9, 2:22 AM
ioeric committed rL336524: [Index] Add indexing support for MACROs..
[Index] Add indexing support for MACROs.
Mon, Jul 9, 1:49 AM
ioeric committed rC336524: [Index] Add indexing support for MACROs..
[Index] Add indexing support for MACROs.
Mon, Jul 9, 1:49 AM
ioeric closed D48961: [Index] Add indexing support for MACROs..
Mon, Jul 9, 1:49 AM
ioeric updated the diff for D48961: [Index] Add indexing support for MACROs..
  • addressed review comments.
Mon, Jul 9, 1:13 AM

Fri, Jul 6

ioeric updated the diff for D49028: [clangd] Support indexing MACROs..
  • Another minor cleanup.
Fri, Jul 6, 8:33 AM
ioeric updated the diff for D49028: [clangd] Support indexing MACROs..
  • Some cleanup.
Fri, Jul 6, 8:27 AM
ioeric added a dependency for D49028: [clangd] Support indexing MACROs.: D48961: [Index] Add indexing support for MACROs..
Fri, Jul 6, 8:22 AM
ioeric added a dependent revision for D48961: [Index] Add indexing support for MACROs.: D49028: [clangd] Support indexing MACROs..
Fri, Jul 6, 8:22 AM
ioeric created D49028: [clangd] Support indexing MACROs..
Fri, Jul 6, 8:19 AM
ioeric updated the diff for D48961: [Index] Add indexing support for MACROs..
  • removed Undefined parameter.
Fri, Jul 6, 5:08 AM
ioeric added inline comments to D48961: [Index] Add indexing support for MACROs..
Fri, Jul 6, 4:53 AM
ioeric updated the diff for D48961: [Index] Add indexing support for MACROs..
  • addressed review comments.
Fri, Jul 6, 4:53 AM
ioeric added a comment to D49012: [clangd] Uprank delcarations when "using q::name" is present in the main file.

Just one caveat regarding locations in addition to Sam's comments :)

Fri, Jul 6, 3:07 AM · Restricted Project
ioeric committed rL336427: [SemaCodeComplete] Expose a method to create CodeCompletionString for macros..
[SemaCodeComplete] Expose a method to create CodeCompletionString for macros.
Fri, Jul 6, 2:48 AM
ioeric committed rC336427: [SemaCodeComplete] Expose a method to create CodeCompletionString for macros..
[SemaCodeComplete] Expose a method to create CodeCompletionString for macros.
Fri, Jul 6, 2:48 AM
ioeric closed D48973: [SemaCodeComplete] Expose a method to create CodeCompletionString for macros..
Fri, Jul 6, 2:48 AM
ioeric updated the diff for D48973: [SemaCodeComplete] Expose a method to create CodeCompletionString for macros..
  • Merged with orgin/master
Fri, Jul 6, 1:37 AM
ioeric added inline comments to D48973: [SemaCodeComplete] Expose a method to create CodeCompletionString for macros..
Fri, Jul 6, 1:29 AM
ioeric updated the diff for D48973: [SemaCodeComplete] Expose a method to create CodeCompletionString for macros..
  • Addressed review comments.
Fri, Jul 6, 1:29 AM
ioeric added a reviewer for D48961: [Index] Add indexing support for MACROs.: sammccall.
Fri, Jul 6, 1:22 AM
ioeric added a comment to D48961: [Index] Add indexing support for MACROs..

Thanks for the review!

Fri, Jul 6, 1:22 AM
ioeric updated the diff for D48961: [Index] Add indexing support for MACROs..
  • Addressed review comments. Expose create a PPCallbacks for indexing macros.
Fri, Jul 6, 1:21 AM

Thu, Jul 5

ioeric added a comment to D48341: [clang-doc] Refactoring mapper to map by scope.

Thanks for the refactoring and the comments! They made it a lot easier to understand the changes.

Thu, Jul 5, 12:06 PM · Restricted Project
ioeric created D48973: [SemaCodeComplete] Expose a method to create CodeCompletionString for macros..
Thu, Jul 5, 7:50 AM
ioeric created D48961: [Index] Add indexing support for MACROs..
Thu, Jul 5, 4:23 AM
ioeric committed rL336321: [clangd] Log sema completion context kind and query scopes. NFC.
[clangd] Log sema completion context kind and query scopes. NFC
Thu, Jul 5, 1:34 AM
ioeric committed rCTE336321: [clangd] Log sema completion context kind and query scopes. NFC.
[clangd] Log sema completion context kind and query scopes. NFC
Thu, Jul 5, 1:34 AM
ioeric closed D48724: [clangd] Log sema completion context kind and query scopes. NFC.
Thu, Jul 5, 1:34 AM
ioeric updated the diff for D48724: [clangd] Log sema completion context kind and query scopes. NFC.

-Rebase

Thu, Jul 5, 1:33 AM
ioeric committed rL336318: [clangd] Treat class constructor as in the same scope as the class in ranking..
[clangd] Treat class constructor as in the same scope as the class in ranking.
Thu, Jul 5, 1:19 AM
ioeric committed rCTE336318: [clangd] Treat class constructor as in the same scope as the class in ranking..
[clangd] Treat class constructor as in the same scope as the class in ranking.
Thu, Jul 5, 1:18 AM
ioeric closed D48933: [clangd] Treat class constructor as in the same scope as the class in ranking..
Thu, Jul 5, 1:18 AM
ioeric updated the diff for D48933: [clangd] Treat class constructor as in the same scope as the class in ranking..
  • Refactored findAnyDecl.
Thu, Jul 5, 1:14 AM

Wed, Jul 4

ioeric accepted D48938: [clangd] Track origins of symbols (various indexes, Sema)..

lgtm. neat!

Wed, Jul 4, 6:59 AM
ioeric created D48933: [clangd] Treat class constructor as in the same scope as the class in ranking..
Wed, Jul 4, 5:37 AM
ioeric committed rCTE336260: [clangd] only ignore collected symbols if TU has uncompilable errors..
[clangd] only ignore collected symbols if TU has uncompilable errors.
Wed, Jul 4, 3:44 AM
ioeric committed rL336260: [clangd] only ignore collected symbols if TU has uncompilable errors..
[clangd] only ignore collected symbols if TU has uncompilable errors.
Wed, Jul 4, 3:44 AM
ioeric committed rL336255: [SemaCodeComplete] Make sure visited contexts are passed to completion results….
[SemaCodeComplete] Make sure visited contexts are passed to completion results…
Wed, Jul 4, 3:06 AM
ioeric committed rC336255: [SemaCodeComplete] Make sure visited contexts are passed to completion results….
[SemaCodeComplete] Make sure visited contexts are passed to completion results…
Wed, Jul 4, 3:06 AM
ioeric closed D48917: [SemaCodeComplete] Make sure visited contexts are passed to completion results handler..
Wed, Jul 4, 3:06 AM
ioeric committed rCTE336253: [clangd] Cleanup unittest: URIs. NFC..
[clangd] Cleanup unittest: URIs. NFC.
Wed, Jul 4, 2:59 AM
ioeric committed rL336253: [clangd] Cleanup unittest: URIs. NFC..
[clangd] Cleanup unittest: URIs. NFC.
Wed, Jul 4, 2:59 AM
ioeric committed rCTE336252: [clangd] Avoid collecting symbols from broken TUs in global-symbol-builder..
[clangd] Avoid collecting symbols from broken TUs in global-symbol-builder.
Wed, Jul 4, 2:48 AM
ioeric committed rL336252: [clangd] Avoid collecting symbols from broken TUs in global-symbol-builder..
[clangd] Avoid collecting symbols from broken TUs in global-symbol-builder.
Wed, Jul 4, 2:48 AM
ioeric closed D48881: [clangd] Avoid collecting symbols from broken TUs in global-symbol-builder..
Wed, Jul 4, 2:48 AM
ioeric added inline comments to D48917: [SemaCodeComplete] Make sure visited contexts are passed to completion results handler..
Wed, Jul 4, 2:46 AM
ioeric updated the diff for D48917: [SemaCodeComplete] Make sure visited contexts are passed to completion results handler..
  • Addressed review comment.
Wed, Jul 4, 2:45 AM
ioeric added inline comments to D48881: [clangd] Avoid collecting symbols from broken TUs in global-symbol-builder..
Wed, Jul 4, 2:30 AM
ioeric added inline comments to D48917: [SemaCodeComplete] Make sure visited contexts are passed to completion results handler..
Wed, Jul 4, 2:20 AM
ioeric updated subscribers of D48441: [clangd] Incorporate transitive #includes into code complete proximity scoring..

Hi Carlos, thanks for letting us know! I sent r336249 to fix the windows
test.

Wed, Jul 4, 2:14 AM
ioeric committed rL336249: Try to fix FileDistance test on windows..
Try to fix FileDistance test on windows.
Wed, Jul 4, 2:13 AM
ioeric committed rCTE336249: Try to fix FileDistance test on windows..
Try to fix FileDistance test on windows.
Wed, Jul 4, 2:13 AM
ioeric created D48917: [SemaCodeComplete] Make sure visited contexts are passed to completion results handler..
Wed, Jul 4, 1:06 AM

Tue, Jul 3

ioeric committed rCTE336203: [clangd] Use default format style and fallback style. NFC.
[clangd] Use default format style and fallback style. NFC
Tue, Jul 3, 7:56 AM
ioeric committed rL336203: [clangd] Use default format style and fallback style. NFC.
[clangd] Use default format style and fallback style. NFC
Tue, Jul 3, 7:56 AM
ioeric created D48881: [clangd] Avoid collecting symbols from broken TUs in global-symbol-builder..
Tue, Jul 3, 7:03 AM
ioeric accepted D47537: [clang-tools-extra] Cleanup documentation routine.

lgtm. Thanks for doing this!

Tue, Jul 3, 6:50 AM · Restricted Project, Restricted Project

Mon, Jul 2

ioeric accepted D48821: [clangd] ClangdServer::codeComplete return CodeCompleteResult, not LSP struct..

lgtm

Mon, Jul 2, 2:57 AM
ioeric accepted D48441: [clangd] Incorporate transitive #includes into code complete proximity scoring..

Again, we might still want to measure the performance impact on files with potentially large #include tree ;)

Mon, Jul 2, 2:14 AM

Fri, Jun 29

ioeric accepted D48762: [clangd] codeComplete returns more structured completion items, LSP. NFC..

lgtm

Fri, Jun 29, 7:41 AM
ioeric added a comment to D48762: [clangd] codeComplete returns more structured completion items, LSP. NFC..

Looks great! Mostly nits.

Fri, Jun 29, 5:45 AM

Thu, Jun 28

ioeric committed rL335874: [clangd] Use log10 instead of the natural logrithm for usage boost..
[clangd] Use log10 instead of the natural logrithm for usage boost.
Thu, Jun 28, 9:56 AM
ioeric committed rCTE335874: [clangd] Use log10 instead of the natural logrithm for usage boost..
[clangd] Use log10 instead of the natural logrithm for usage boost.
Thu, Jun 28, 9:55 AM
ioeric created D48724: [clangd] Log sema completion context kind and query scopes. NFC.
Thu, Jun 28, 7:55 AM
ioeric added inline comments to D48441: [clangd] Incorporate transitive #includes into code complete proximity scoring..
Thu, Jun 28, 4:53 AM

Tue, Jun 26

ioeric committed rCTE335598: [clangd] Use default clang-format styles..
[clangd] Use default clang-format styles.
Tue, Jun 26, 5:56 AM
ioeric committed rL335598: [clangd] Use default clang-format styles..
[clangd] Use default clang-format styles.
Tue, Jun 26, 5:53 AM