Page MenuHomePhabricator

tom-anders (Tom Praschan)
User

Projects

User does not belong to any projects.

User Details

User Since
Aug 16 2021, 1:01 PM (51 w, 5 d)

Recent Activity

Wed, Aug 10

tom-anders committed rG15bf2aa44aa1: [clangd][test] Fix error message in SerializationTest.BinaryConversions (authored by tom-anders).
[clangd][test] Fix error message in SerializationTest.BinaryConversions
Wed, Aug 10, 11:50 PM · Restricted Project, Restricted Project

Sun, Jul 17

tom-anders abandoned D129971: [clangd][WIP] Add doxygen parsing for Hover.

Sorry, meant to make this a draft

Sun, Jul 17, 11:39 AM · Restricted Project, Restricted Project, Restricted Project
tom-anders requested review of D129971: [clangd][WIP] Add doxygen parsing for Hover.
Sun, Jul 17, 11:37 AM · Restricted Project, Restricted Project, Restricted Project

Jul 10 2022

tom-anders added a comment to D128826: Go-to-type on smart_ptr<Foo> now also shows Foo.

@sammccall I think you can merge this for me now (and also https://reviews.llvm.org/D128202)

Jul 10 2022, 6:25 AM · Restricted Project, Restricted Project

Jun 29 2022

tom-anders updated the diff for D128826: Go-to-type on smart_ptr<Foo> now also shows Foo.
  • Remove obsolete comment
  • Return empty vector when type isNull()
  • Add comment about why we don't deduplicate results
  • Use out-parameter in unwrapFindType to make structure more consistent
Jun 29 2022, 1:18 PM · Restricted Project, Restricted Project
tom-anders added a comment to D128826: Go-to-type on smart_ptr<Foo> now also shows Foo.

So here's what I thought: Say you have a variable that's a smart pointer and trigger textDocument/typeDefinition on it and there's only a single result that takes you straight to the type. In this case, you might not even know that the type is actually wrapped inside a smart pointer and might make wrong assumptions about the code. Of course you could've done a Hover beforehand, but in the case where there are 2 results, you don't need that extra step.

Yeah. There's no guarantee the type in question is well-represented by a decl, but this is a bit confusing.
I think it might still be worth it for specific types like unique_ptr<Foo>, but for vector<Foo> it's too surprising even if you want Foo 99% of the time.

Right, maybe it would make sense to go straight to T for declarations, but include smart_ptr when it's used in an expression? But that's something for a follow-up patch.

Hmm, is the distinction that the type is written in the declaration (not auto)? I'm not sure it's worth specializing behavior for that scenario too much, when it's not too hard to go-to-definition on the specific token you want.
The chance of guessing right has to be weighed against keeping the behavior predictable and comprehensible...

Yes that's what I meant, but I agree with you here.

Jun 29 2022, 1:16 PM · Restricted Project, Restricted Project
tom-anders added inline comments to D128826: Go-to-type on smart_ptr<Foo> now also shows Foo.
Jun 29 2022, 11:45 AM · Restricted Project, Restricted Project
tom-anders added inline comments to D128826: Go-to-type on smart_ptr<Foo> now also shows Foo.
Jun 29 2022, 11:37 AM · Restricted Project, Restricted Project
tom-anders added a comment to D128826: Go-to-type on smart_ptr<Foo> now also shows Foo.

I'm using this with neovim's builtin LSP, where when there are multiple results it takes me to the first one, and also opens the quickfix window that shows all results. But I can see how the current implementation could be annoying with other editors.

(Interesting, I'm using the same and find it annoying: it focuses the quickfix window and that's disruptive enough that I never noticed the cursor also moved. But https://github.com/neovim/neovim/pull/14878 lists some workarounds)

(Ah, I also forgot that I'm not actually using quickfix, but rather https://github.com/folke/trouble.nvim, which also gets focused but has a preview feature)

Jun 29 2022, 11:32 AM · Restricted Project, Restricted Project
tom-anders added a comment to D128826: Go-to-type on smart_ptr<Foo> now also shows Foo.

Thanks! I just have a question if this behavior should be even "stronger":

In most editors, if you return one result you go straight there, if you return two results you get a menu.
A menu is significantly worse than going straight to the right result - the UI is clunkier and the choice interrupts your workflow.

So my question is, do you think that we should *only* return the pointee in these cases?

(It's possible to change this later, but I'd like to take a reasonable guess as this is forcing an API change from Type -> vector<Type>)
cc @kadircet

Jun 29 2022, 8:33 AM · Restricted Project, Restricted Project
tom-anders added inline comments to D128826: Go-to-type on smart_ptr<Foo> now also shows Foo.
Jun 29 2022, 7:40 AM · Restricted Project, Restricted Project
tom-anders added reviewers for D128826: Go-to-type on smart_ptr<Foo> now also shows Foo: nridge, sammccall.
Jun 29 2022, 7:37 AM · Restricted Project, Restricted Project
tom-anders requested review of D128826: Go-to-type on smart_ptr<Foo> now also shows Foo.
Jun 29 2022, 7:37 AM · Restricted Project, Restricted Project

Jun 28 2022

tom-anders updated the diff for D128202: [clangd] Include "final" when printing class declaration.

Check for FinalAttr instead of using isEffectivelyFinal()

Jun 28 2022, 10:04 AM · Restricted Project, Restricted Project, Restricted Project

Jun 20 2022

tom-anders added inline comments to D128202: [clangd] Include "final" when printing class declaration.
Jun 20 2022, 7:17 AM · Restricted Project, Restricted Project, Restricted Project
tom-anders added reviewers for D128202: [clangd] Include "final" when printing class declaration: nridge, sammccall.
Jun 20 2022, 7:06 AM · Restricted Project, Restricted Project, Restricted Project
tom-anders requested review of D128202: [clangd] Include "final" when printing class declaration.
Jun 20 2022, 7:05 AM · Restricted Project, Restricted Project, Restricted Project

Sep 8 2021

tom-anders added a comment to D108320: Add semantic token modifier for non-const reference parameter.

I’d say commit as is and have a look at the SmallVector -> unit32_t thing together with the std::map issue afterwards

Sep 8 2021, 1:24 AM · Restricted Project

Sep 7 2021

tom-anders added a comment to D108320: Add semantic token modifier for non-const reference parameter.

Yeah, I‘d need someone to commit this for me.

Sep 7 2021, 11:56 PM · Restricted Project

Aug 28 2021

tom-anders updated the diff for D108320: Add semantic token modifier for non-const reference parameter.
  • Use llvm::SmallVector, add more FIXME, remove old commented out test
Aug 28 2021, 4:02 AM · Restricted Project
tom-anders added inline comments to D108320: Add semantic token modifier for non-const reference parameter.
Aug 28 2021, 4:00 AM · Restricted Project

Aug 23 2021

tom-anders added a comment to D108320: Add semantic token modifier for non-const reference parameter.

@sammccall @nridge Thanks for the positive feedback, let me know if I missed any of your suggestions!

Aug 23 2021, 12:46 PM · Restricted Project
tom-anders updated the diff for D108320: Add semantic token modifier for non-const reference parameter.
  • clang-format once again
Aug 23 2021, 12:43 PM · Restricted Project
tom-anders updated the diff for D108320: Add semantic token modifier for non-const reference parameter.
  • Remove accidentally added empty line
Aug 23 2021, 12:40 PM · Restricted Project
tom-anders updated the diff for D108320: Add semantic token modifier for non-const reference parameter.
  • Apply suggested renaming and fix nits
  • Use a map for extra modifiers instead of abusing conflict resolution
  • Use ArrayRef instead of callback
  • Add FIXME for handling CXXOperatorCallExpr
  • Use getLocation() instead of getBeginLoc()
  • Add FIXME for unwrapping operators
  • Iterate over FunctionProtoType instead of FunctionDecl
  • Adjust highlighting condition and revise test cases
Aug 23 2021, 12:37 PM · Restricted Project
tom-anders added inline comments to D108320: Add semantic token modifier for non-const reference parameter.
Aug 23 2021, 12:35 PM · Restricted Project

Aug 19 2021

tom-anders added inline comments to D108320: Add semantic token modifier for non-const reference parameter.
Aug 19 2021, 12:10 PM · Restricted Project
tom-anders planned changes to D108320: Add semantic token modifier for non-const reference parameter.
Aug 19 2021, 11:31 AM · Restricted Project

Aug 18 2021

tom-anders added a reviewer for D108320: Add semantic token modifier for non-const reference parameter: nridge.
Aug 18 2021, 3:17 PM · Restricted Project
tom-anders added a reviewer for D108320: Add semantic token modifier for non-const reference parameter: sammccall.
Aug 18 2021, 1:34 PM · Restricted Project
tom-anders requested review of D108320: Add semantic token modifier for non-const reference parameter.
Aug 18 2021, 12:36 PM · Restricted Project