[clangd] option to disable adding parenthesis for function with auto-completion

[ghost]

I personally am not a huge fan of having this happening. I would really love to see this option available!

What I mean is kind of like this option: --function-arg-placeholders=0. This disabling the automatic filling of argument slots. In the same way I'd love to see clangd not adding the parenthesis when auto-completion is used.

[llvmbot]

@llvm/issue-subscribers-clangd

[HighCommander4]

clangd/clangd#1252 is another request for a configuration option related to what text is inserted when when selecting a completion proposal.

Perhaps it would be appropriate to add both options in the Completion section of the config file?

Should --function-arg-placeholders be deprecated in favour of a config file option as well, to have everything in one place?

[lhmouse]

Is there any progress on this issue? This has always been disrupting and annoying for me, as it always duplicates opening parentheses unlike any other editor.

[HighCommander4]

Here's a concrete proposal for a config file syntax that would address this request, clangd/clangd#1252, and supersede --function-arg-placeholders: add a new key under Completion called ArgumentLists, with the following supported values:

Value	Description	Function Example	Template Example
None	Nothing inserted in argument list position	std::atan2	std::array
Delimiters	Empty pair of delimiters inserted	std::atan2()	std::array<>
NamePlaceholders	Delimiters and placeholder arguments are inserted. The placeholders are just the names of the parameters.	std::atan2(y, x)	std::array<T, N>
FullPlaceholders	Delimiters and placeholder arguments are inserted. The placeholders are the names and types of the parameters. (Current behaviour)	std::atan2(float y, float x)	std::array<typename T, size_t N>

For backwards compatibility:

• the default would be FullPlaceholders (matching the current default behaviour)
• --function-arg-placeholders would continue to be supported, with:
  • --function-arg-placeholders=0 being an alias for ArgumentLists: Delimiters
  • --function-arg-placeholders=1 being an alias for ArgumentLists: FullPlaceholders

If both --function-arg-placeholders and ArgumentLists are specified, ArgumentLists would take precedence (I believe that's the behaviour that falls out of FlagsConfigProvider in similar cases).

Thoughts?

[MK-Alias]

This would fix all my annoyances I have with clangd in VScode. really hope this gets approved and implemented (soon).! I really love clangd, it really improves my work but I need the None option to 100% happy with it.! Keeping my fingers crossed!

[lhmouse]

The idea sounds great to me, too.

[zyd2001]

It sounds good to me.

[MK-Alias]

* With a friendly smile on his face, and sympathetic tone in his voice, MK-Alias is asking *

This issue/thing/ticket is over a year old, and my own ticket #390 on this, is about two years old. I'm kind of wondering how this is going..!?

[HighCommander4]

I haven't had time to work on this, sorry.

I know I comment on clangd issues frequently, but just in case anyone has misconceptions about my relationship to the project: no one is employing me to contribute to clangd. I do it in my spare time, and the amount of spare time I have is limited and variable over time.

Recently, I've been using that limited time to focus on what I see as the highest-impact contributions I can make:

• Helping to resolve severe issues that prevent people from using clangd altogether, such as crashes and runaway memory usage
• Reviewing code contributions from others

Improvements in other areas will need to come from other members of the community stepping up to contribute to things they care about.

[MK-Alias]

Thanks for your reply, and elaboration! I did kind of thought that most where volunteering. Maybe I'll write my own patch, the work doesn't seem that difficult, I'm just not familiar with the codebase, and also don't have much time for it.

[HighCommander4]

I'm happy to provide pointers to the relevant code and other guidance as needed :)

[MK-Alias]

I've been at it for some part of the day, and got a patch which already implements everything except for the NamePlaceholders variant. All others, including [now deprecated] legacy "0" and "1" work with this patch.

The problem is, that I don't know how to implement NamePlaceholders, because I've got no "example" for this. Or simply put, I don't know how to get rid of the storage/type/class parameter of the snippet in std::string summarizeSnippet() const

here is the patch: placeholders-patch-alpha.patch

You can apply it to the LLVM source with: git apply <path_to>/placeholders-patch-alpha.patch

In its current state, this patch is already very usable for myself, because I only need the None option. But I've anyone has any great idea's on how to implement the NamePlaceholders, I'm all ears. Also, perhaps also implement a version which only inserts the opening '(' or '<' OpenOnly ??? (just a thought)

Edit: here's a screenshot of the help text regarding the new features:

[HighCommander4]

The problem is, that I don't know how to implement NamePlaceholders, because I've got no "example" for this. Or simply put, I don't know how to get rid of the storage/type/class parameter of the snippet in std::string summarizeSnippet() const

Good question. I would say, rather than trying to remove the type name for a parameter placeholder string that already contains it, to implement NamePlaceholders we should ensure that the placeholder string does not contain the type name to begin with.

Tracing through the code a bit, the snippet string comes from here, which is set here (for code completion results coming from the AST); inside that function, the contents of a placeholder for a single argument comes from here, where the source string (Chunk.Text) already contains the type name.

So, to get rid of the type name, we need to look earlier, where the CodeCompletionString object is created. That happens here, which calls into code in clang/lib/Sema, which ultimately adds the type name to the contents of a function parameter chunk here. (The operation takes the parameter name, which was set here, and "decorates" it with the type name; the type name is not always a pure prefix of the parameter name, as there may be cases like int (*function_ptr)(int) where the type is "around" the name function_ptr.)

So, to implement NamePlacholders, we need to propagate a boolean flag into FormatFunctionParameter() that controls whether the type name is added.

(An interesting question is what to do about code completion results coming from the index. For those, the snippet is computed at the time the index is built, and stored in the index. Therefore, the above code change will take effect for those too, but it will be based on the value of the setting at the time the index was built. If you change the setting, the format of the snippets for results coming from the index will not be updated until the index is rebuilt. Not sure what to do about that... it may be a wart we just need to live with.)

If all that sounds involved, I'd say it's fine to just implement the other three options first, and leave NamePlaceholders for a follow-up patch; after all, just being able to choose None opens up new possibilities and thus is a useful change.

[MK-Alias]

So simply put, the framework in it's current state doesn't support the removal of the storage/type. So to implement NamePlaceholders we can either:

• update the framework to support it
• reformat the snippet output

Reformatting seems as too much of a workaround, and if the framework [ever] gets updated, the reformatted functionality might break, which is then an unsuspected side-effect. So updating the framework seems like the better choice. However, putting a boolean on the functionality just to accommodate our placeholder issue, seems a bit specific.

If all that sounds involved, I'd say it's fine to just implement the other three options first, and leave NamePlaceholders for a follow-up patch; after all, just being able to choose None opens up new possibilities and thus is a useful change.

other than that, time is also an issue. It will probably takes some time to figure everything out, which I rater not spend right now. Mostly for that reason I'm leaning towards dropping NamePlaceholders. If the snippet code ever gets updated, the placeholders functionality can easily be updated with it.

I can however, implement a OpenOnly version, which will only output the opening '(' or '<'. eg: void foo(

Is that a good idea?? And is the help text (see earlier screenshot) to everyone's liking?

[HighCommander4]

Mostly for that reason I'm leaning towards dropping NamePlaceholders. If the snippet code ever gets updated, the placeholders functionality can easily be updated with it.

+1

I can however, implement a OpenOnly version, which will only output the opening '(' or '<'. eg: void foo(

Is that a good idea??

Unless this is something you plan to use yourself, I would wait until someone asks for this.

And is the help text (see earlier screenshot) to everyone's liking?

Looks generally good to me. I may have suggestions for small tweaks but those are easier to make in the form of comments on a PR.

[MK-Alias]

Okay, I've created a new "beta" patch: placeholders-patch-beta.patch

After playing around with it I really think that an Open should really be a part of this. It's something I might actually use myself in favor of None. But I'm sure that there are people who would prefer this.

This patch has 2 minor issues with vscode/vscode-clangd:

• None: after completion an error squiggly line will occur on the identifier. Would be slightly nicer if it wouldn't.
• Open: the signature help hover doesn't spawn. It seems that Sam's Patch from #398 needs to be reconfigured for this.

This patch should be fully applicable, other than perhaps taking a last critical look at the help description texts.., and of course, feel free to change/reconfigure things where deemed necessary. hope to see it implemented soon! Until then I'll use my local dev copy.

[HighCommander4]

Okay, I've created a new "beta" patch: placeholders-patch-beta.patch

Could you submit it as a pull request please?

After playing around with it I really think that an Open should really be a part of this. It's something I might actually use myself in favor of None. But I'm sure that there are people who would prefer this.

Ok, sounds reasonable to me. Name suggestion: OpenDelimiter (for consistency with Delimiters).

[MK-Alias]

For tree hours I've tried to fork -> clone -> patch -> commit -> push the patch and make a pull request. And I've failed. Github - in my sole opinion - has made it WAY to cumbersome to do these things. This was not a nice user experience at all. I think git itself is already overly complicated - I'm just using it because it became the "unwritten standard" - but Github is next level frustration. This rabbit hole is full of:

• authentication problems
• never seen before error messages
• git config dependencies (doing different things based upon config)
• different behavior based upon flags (eg: git pull, vs git pull --rebase)
• different behavior based upon you OS and OS settings
• OS Credential managers
• a thing called: git-credential-manager

^^^ these things all don't like each other, and its up to me to make them work together ^^^
Development tools should make my life easier, not the other way around, and I might even have ruined some of my config with trying to cope with this.

So to answer your question:

Could you submit it as a pull request please?

Despite my trying, unfortunately, no!

Here is the last updated version of the patch. I basically just renamed Open to OpenDelimiter as you suggested. And changed a comment: placeholders-patch-gamma.patch

It would be my honer if it still gets integrated into clangd - and by all means feel free to modify/reconfigure it as you see fit. However, I'm done with the whole "Github: pull request" fetch quest for now.... for which I'm sorry!

[lhmouse]

@MK-Alias

The simplest way to set up Git credential is to use Git bash. You generate a key pair with ssh-keygen (just press Enter all the way). Depending on the algorithm being used, they are stored as either ~/.ssh/id_rsa & ~/.ssh/id_rsa.pub or ~/.ssh/id_ed25519 & ~/.ssh/id_ed25519.pub. The file whose name ends with .pub is the public key as a text file, then you need to go to your profile and add it to SSH keys (link: https://github.com/settings/keys).

Once you have your SSH key set up, you can clone all GitHub repos using SSH protocol. DO NOT USE HTTPS, as HTTPS is read-only:

git clone [email protected]:<user>/<repo>.git

If the server is asking you for a password for the git account, do not enter a password; it indicates that your public key has not been set up correctly.

This will allow you to push commits to your own repo. Once you have pushed your commits, you can file a Pull Request at https://github.com/llvm/llvm-project/compare . You need to click compare across forks to select your own repo as the source.

[MK-Alias]

@lhmouse

Thanks for the guide, and with doing so I was able to submit the pull request!

During my previous 3 hour adventure, there was a little voice in me that suggested to try ssh . That voice wasn't loud enough, glad that your voice was!

Thanks, buddy!