Page MenuHomePhabricator

simark (Simon Marchi)
User

Projects

User does not belong to any projects.

User Details

User Since
Mar 15 2017, 11:57 AM (90 w, 6 d)

Recent Activity

Wed, Dec 5

simark added inline comments to D55250: [clangd] Enhance macro hover to see full definition.
Wed, Dec 5, 7:27 AM

Nov 5 2018

simark added a comment to D54077: [clangd] Implemented DraftFileSystem.

I'm in yet another camp: I carefully save when I have something that is correct enough syntax, so I only want errors from with changes from the exact file I'm editing and the rest of the files in saved state.

Nov 5 2018, 7:26 AM

Oct 16 2018

simark committed rL344614: Remove possibility to change compile database path at runtime.
Remove possibility to change compile database path at runtime
Oct 16 2018, 8:57 AM
simark committed rCTE344614: Remove possibility to change compile database path at runtime.
Remove possibility to change compile database path at runtime
Oct 16 2018, 8:57 AM
simark closed D53220: Remove possibility to change compile database path at runtime.
Oct 16 2018, 8:57 AM
simark updated the diff for D53220: Remove possibility to change compile database path at runtime.

Address comments

Oct 16 2018, 8:27 AM
simark added inline comments to D53220: Remove possibility to change compile database path at runtime.
Oct 16 2018, 8:27 AM

Oct 15 2018

simark added inline comments to D53220: Remove possibility to change compile database path at runtime.
Oct 15 2018, 8:54 AM

Oct 12 2018

simark abandoned D51725: Allow un-setting the compilation database path.
Oct 12 2018, 2:43 PM
simark added a comment to D51725: Allow un-setting the compilation database path.

Thanks for your input. I have opened https://reviews.llvm.org/D53220, which removes that option.

Oct 12 2018, 2:43 PM
simark created D53220: Remove possibility to change compile database path at runtime.
Oct 12 2018, 2:40 PM

Oct 4 2018

simark added a comment to D52311: [clangd] Add support for hierarchical documentSymbol.

I just tried this, this looks very promising! It should help build our outline view in a much more robust way than we do currently.

Oct 4 2018, 2:40 PM
simark added a comment to D51725: Allow un-setting the compilation database path.

So I revisited this today. In the end, restarting clangd in our IDE works well. It should be merged anytime soon:

Oct 4 2018, 12:37 PM

Sep 20 2018

simark added a comment to D52311: [clangd] Add support for hierarchical documentSymbol.

Ohh awesome, I didn't know the LSP supported that. I'll try it it Theia when I have time.

Sep 20 2018, 10:04 AM

Sep 13 2018

simark added a comment to D51725: Allow un-setting the compilation database path.

If this setting exposed directly the users in Theia or is it something that is exposed via a custom UI (e.g. choosing named build configs or something similar)?

Sep 13 2018, 6:06 AM

Sep 12 2018

simark added a comment to D51725: Allow un-setting the compilation database path.

Wow, this is getting somewhat complicated.

Have you considered rerunning clangd whenever someone changes an option like that?
Would that be much more complicated on your side?

Not opposed to having an option too, just want to be aware of the costs involved on your end.

Sep 12 2018, 10:10 AM
simark added a comment to D51725: Allow un-setting the compilation database path.

Not sure who should review this, please feel free to add anybody who would be appropriate.

Sep 12 2018, 8:16 AM
simark added a reviewer for D51725: Allow un-setting the compilation database path: ilya-biryukov.
Sep 12 2018, 8:14 AM

Sep 6 2018

simark updated the diff for D51725: Allow un-setting the compilation database path.

Fix formatting.

Sep 6 2018, 3:42 AM
simark created D51725: Allow un-setting the compilation database path.
Sep 6 2018, 3:30 AM

Aug 29 2018

simark added inline comments to D51311: (WIP) Type hierarchy.
Aug 29 2018, 8:06 AM

Aug 28 2018

simark added inline comments to D51311: (WIP) Type hierarchy.
Aug 28 2018, 8:02 AM

Aug 27 2018

simark planned changes to D51311: (WIP) Type hierarchy.
Aug 27 2018, 8:47 AM
simark created D51311: (WIP) Type hierarchy.
Aug 27 2018, 8:46 AM

Aug 23 2018

simark added inline comments to D51159: [FileManager] Do not call 'real_path' in getFile()..
Aug 23 2018, 1:13 PM
simark added inline comments to D51159: [FileManager] Do not call 'real_path' in getFile()..
Aug 23 2018, 12:33 PM
simark added inline comments to D51159: [FileManager] Do not call 'real_path' in getFile()..
Aug 23 2018, 11:09 AM
simark added inline comments to D51159: [FileManager] Do not call 'real_path' in getFile()..
Aug 23 2018, 10:35 AM

Aug 10 2018

simark added inline comments to D48687: [clangd] Avoid duplicates in findDefinitions response.
Aug 10 2018, 3:29 PM
simark committed rL339483: [clangd] Avoid duplicates in findDefinitions response.
[clangd] Avoid duplicates in findDefinitions response
Aug 10 2018, 3:28 PM
simark committed rCTE339483: [clangd] Avoid duplicates in findDefinitions response.
[clangd] Avoid duplicates in findDefinitions response
Aug 10 2018, 3:28 PM
simark closed D48687: [clangd] Avoid duplicates in findDefinitions response.
Aug 10 2018, 3:28 PM
simark updated the diff for D48687: [clangd] Avoid duplicates in findDefinitions response.

Latest update.

Aug 10 2018, 1:54 PM
simark added a comment to D48687: [clangd] Avoid duplicates in findDefinitions response.

Looks good with a few nits. Looks like you didn't update the patch correctly. You have marked comments done, but your code doesn't get changed accordingly. Please double check with it.

I tried your patch and it did fix the duplicated issue I encountered before. Thanks for fixing it!

Aug 10 2018, 1:51 PM
simark added inline comments to D48687: [clangd] Avoid duplicates in findDefinitions response.
Aug 10 2018, 8:28 AM

Aug 9 2018

simark updated the diff for D48687: [clangd] Avoid duplicates in findDefinitions response.

Replace

Aug 9 2018, 12:08 PM
simark added a comment to D48687: [clangd] Avoid duplicates in findDefinitions response.

Sorry for the delay. I thought there was a dependent patch of this patch, but it seems resolved, right?

A rough round of review.

Aug 9 2018, 12:06 PM

Aug 8 2018

simark updated the diff for D48687: [clangd] Avoid duplicates in findDefinitions response.

Formatting.

Aug 8 2018, 12:09 PM
simark updated the diff for D48687: [clangd] Avoid duplicates in findDefinitions response.

New version. I tried to share the code between this and SymbolCollector, but
didn't find a good clean way. Do you have concrete suggestions on how to do
this? Otherwise, would it be acceptable to merge it as-is?

Aug 8 2018, 11:32 AM

Aug 6 2018

simark committed rL339063: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the….
[VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the…
Aug 6 2018, 2:49 PM
simark committed rC339063: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the….
[VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the…
Aug 6 2018, 2:49 PM
simark closed D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name.
Aug 6 2018, 2:48 PM
simark added a comment to D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name.

I see, thanks for the explanation.

LGTM for the update revision, given we have confirmation the tests pass on Windows.

Aug 6 2018, 2:47 PM

Aug 5 2018

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

Both check-clang and check-clang-tools pass successfully for me on Windows with the patch.

Aug 5 2018, 6:39 PM

Aug 3 2018

simark added a comment to D50255: Add test for changing build configuration.

Thanks! This LGTM.

Aug 3 2018, 12:41 PM
simark committed rCTE338914: [clangd] Add test for changing build configuration.
[clangd] Add test for changing build configuration
Aug 3 2018, 12:40 PM
simark committed rL338914: [clangd] Add test for changing build configuration.
[clangd] Add test for changing build configuration
Aug 3 2018, 12:40 PM
simark closed D50255: Add test for changing build configuration.
Aug 3 2018, 12:40 PM
simark updated the diff for D50255: Add test for changing build configuration.

The tests now work on Windows, I added some path adjustment steps.

Aug 3 2018, 10:59 AM
simark requested review of D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name.

This revision got 'reopened' and is now in the list of accepted revision. Should we close it again?

Aug 3 2018, 9:24 AM
simark added a comment to D50255: Add test for changing build configuration.

This was not tested on windows yet.

Aug 3 2018, 7:43 AM
simark added a reviewer for D50255: Add test for changing build configuration: arphaman.
Aug 3 2018, 7:43 AM
simark created D50255: Add test for changing build configuration.
Aug 3 2018, 7:42 AM

Aug 2 2018

simark added a comment to D50154: [clangd] capitalize diagnostic messages.

Capitalizing sounds nice. +1 to just do it without an option.

My favorite essay on this is
http://neugierig.org/software/blog/2018/07/options.html

Aug 2 2018, 6:51 AM
simark added a comment to D50154: [clangd] capitalize diagnostic messages.

That's fine with me. I'll try it and time will tell if I like it or not :).

Aug 2 2018, 5:19 AM

Aug 1 2018

simark updated the diff for D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name.

Add -Wno-nonportable-include-path to test/PCH/case-insensitive-include.c

Aug 1 2018, 1:49 PM
simark reopened D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name.
Aug 1 2018, 1:07 PM
simark updated subscribers of D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name.

@eric_niebler, question for you:

Aug 1 2018, 10:52 AM
simark added a comment to D49833: [clangd] Receive compilationDatabasePath in 'initialize' request.

Thanks, done.

Aug 1 2018, 4:30 AM
simark committed rCTE338518: [clangd] Receive compilationDatabasePath in 'initialize' request.
[clangd] Receive compilationDatabasePath in 'initialize' request
Aug 1 2018, 4:29 AM
simark committed rL338518: [clangd] Receive compilationDatabasePath in 'initialize' request.
[clangd] Receive compilationDatabasePath in 'initialize' request
Aug 1 2018, 4:29 AM
simark closed D49833: [clangd] Receive compilationDatabasePath in 'initialize' request.
Aug 1 2018, 4:29 AM

Jul 31 2018

simark updated the diff for D49833: [clangd] Receive compilationDatabasePath in 'initialize' request.

Address Ilya's comment (add an indirection for initializationOptions).

Jul 31 2018, 1:07 PM
simark commandeered D49833: [clangd] Receive compilationDatabasePath in 'initialize' request.
Jul 31 2018, 9:45 AM
simark added a comment to D49833: [clangd] Receive compilationDatabasePath in 'initialize' request.

@simark would you mind finishing this patch and committing it? I won't be able to finish it given that I started it at a different company, etc.

Jul 31 2018, 9:45 AM

Jul 26 2018

simark committed rL338057: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the….
[VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the…
Jul 26 2018, 11:55 AM
simark committed rC338057: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the….
[VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the…
Jul 26 2018, 11:55 AM
simark closed D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name.
Jul 26 2018, 11:55 AM
simark updated the diff for D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name.

I think this addresses all of Ilya's comments.

Jul 26 2018, 5:36 AM
simark added inline comments to D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name.
Jul 26 2018, 5:36 AM

Jul 25 2018

simark added a comment to D49783: [clangd] Do not rebuild AST if inputs have not changed.

Thanks, that's simple and efficient. I'll update D49267 (to call reparseOpenFiles once again) once this is merged.

Jul 25 2018, 1:01 PM
simark abandoned D49804: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name (take 2).
Jul 25 2018, 9:08 AM
simark requested review of D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name.
Jul 25 2018, 9:08 AM
simark updated the diff for D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name.

Fix tests on Windows

Jul 25 2018, 9:08 AM
simark reopened D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name.

It would make it easier for your reviewers to look at the new changes if
you just reopen this patch and update the diff :)

Jul 25 2018, 9:06 AM
simark added a comment to D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name.

I uploaded a new version of this patch here:
https://reviews.llvm.org/D49804

Jul 25 2018, 8:49 AM
simark created D49804: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name (take 2).
Jul 25 2018, 8:49 AM

Jul 24 2018

simark added inline comments to D49267: [clangd] Watch for changes in compile_commands.json.
Jul 24 2018, 7:54 AM
simark added a comment to D49267: [clangd] Watch for changes in compile_commands.json.

The approach is not ideal, but may be a good middle ground before we figure out how we approach file watching in clangd. Note that there are other things that won't force the updates currently, e.g. changes to the included headers won't cause rebuilds of the source files until user modifies them. And those are much more frequent than changes to compile_commands.json, so they seem like a more pressing problem.

Jul 24 2018, 5:29 AM

Jul 23 2018

simark added a comment to D49267: [clangd] Watch for changes in compile_commands.json.

Thanks for putting up this change! It can be really annoying that clangd does not pick up the compile commands that got updated.

A few things of the top of my head on why we might want to punt on using the LSP watches:

  • File watching may not be implemented in the language server at all. That's certainly the case for our hacked-up ycmd<->lsp bridge.
Jul 23 2018, 10:15 AM
simark committed rCTE337697: [clangd] Fix category in clangd-vscode's package.json.
[clangd] Fix category in clangd-vscode's package.json
Jul 23 2018, 7:32 AM
simark committed rL337697: [clangd] Fix category in clangd-vscode's package.json.
[clangd] Fix category in clangd-vscode's package.json
Jul 23 2018, 7:32 AM
simark closed D49253: [clangd] Fix category in clangd-vscode's package.json.
Jul 23 2018, 7:32 AM

Jul 17 2018

simark committed rL337284: [Tooling] Add operator== to CompileCommand.
[Tooling] Add operator== to CompileCommand
Jul 17 2018, 7:18 AM
simark committed rC337284: [Tooling] Add operator== to CompileCommand.
[Tooling] Add operator== to CompileCommand
Jul 17 2018, 7:18 AM
simark closed D49265: [Tooling] Add operator== to CompileCommand.
Jul 17 2018, 7:18 AM
simark closed D49265: [Tooling] Add operator== to CompileCommand.
Jul 17 2018, 7:18 AM

Jul 16 2018

simark updated the diff for D49265: [Tooling] Add operator== to CompileCommand.

Add tests for both == and !=.

Jul 16 2018, 3:38 PM
simark added a comment to D49265: [Tooling] Add operator== to CompileCommand.

In theory you'd need separate tests for op== and op!= returning false (currently all the tests would pass if both implementations returned true in all cases), but not the biggest deal.

Jul 16 2018, 3:17 PM
simark updated the diff for D49265: [Tooling] Add operator== to CompileCommand.
  • Add test
  • Make operator== a function instead of method
  • Add operator!= (so I can use EXPECT_NE in the test, and because it may be useful in general)
Jul 16 2018, 1:42 PM
simark added a comment to D49265: [Tooling] Add operator== to CompileCommand.

Any chance this can/should be unit tested? (also, in general (though might
not matter in this instance), symmetric operators like == should be
implemented as non-members (though they can still be friends and if they
are, can be defined inline in the class definition as a member could be),
so any implicit conversions apply equally to the LHS as the RHS of the
expression)

Jul 16 2018, 11:50 AM

Jul 12 2018

simark updated the diff for D49267: [clangd] Watch for changes in compile_commands.json.

Remove unintended changes

Jul 12 2018, 3:21 PM
simark added a comment to D49267: [clangd] Watch for changes in compile_commands.json.

Note, D49265 in clang is a prerequisite.

Jul 12 2018, 3:19 PM
simark added a reviewer for D49267: [clangd] Watch for changes in compile_commands.json: malaperle.
Jul 12 2018, 2:13 PM
simark created D49267: [clangd] Watch for changes in compile_commands.json.
Jul 12 2018, 2:12 PM
simark created D49265: [Tooling] Add operator== to CompileCommand.
Jul 12 2018, 1:48 PM
simark created D49260: [clangd] JSONTracer: flush after writing event.
Jul 12 2018, 11:26 AM
simark updated the diff for D49253: [clangd] Fix category in clangd-vscode's package.json.

Oops.

Jul 12 2018, 11:26 AM
simark updated the diff for D49253: [clangd] Fix category in clangd-vscode's package.json.
  • [clangd] JSONTracer: flush after writing event
Jul 12 2018, 10:34 AM
simark created D49253: [clangd] Fix category in clangd-vscode's package.json.
Jul 12 2018, 10:29 AM