[RFC] Coding style: drop comments for parameters in calls (original) (raw)

There is a lot of code like

      ChosenTailFoldingStyle = std::make_pair(
          TTI.getPreferredTailFoldingStyle(/*IVUpdateMayOverflow=*/true),
          TTI.getPreferredTailFoldingStyle(/*IVUpdateMayOverflow=*/false));

in the tree. However, with clangd argument-hint overlays, comments like /*IVUpdateMayOverflow=*/ are unnecessary, as the hinting overlay displays the argument name anyway. While I understand that these are only displayed in an editor with clangd enabled, don’t most of the contributors have this set up already?

I propose that we drop the redundant argument-comments going forward.

A related proposal is to check in a .vscode/extensions.json into the tree recommending the clangd extension to make it easy for new contributors to discover. I don’t know if there’s an equivalent setting in Vim/Emacs, but if there is, we can recommend it there too.

jh7370 June 4, 2025, 1:55pm 2

Strong -1. I think your assumption that most contributors have clangd set up is likely false and even if it is true, it would have to be a very high proportion of developers for this to warrant removal of the code comments. Certainly, I don’t have it enabled and I have no intention to do so. I suspect most new contributors don’t either. Furthermore, git and github don’t. It’s useful for reviewing and analysis in these environments to have these sorts of comments.

Endill June 4, 2025, 2:00pm 3

I definitely want to be able to see those comments in setups without a language server, e.g. in a GitHub review UI. So I strongly oppose to this.

I don’t.

FWIW, even if I had an editor with clangd enabled, I’d find it harder to review code in GitHub’s UI without those kinds of comments.

I think we should continue to encourage use of those kinds of comments.

Endill June 4, 2025, 2:03pm 5

One possible refactoring to get rid of those comments would be to convert tons of boolean parameters we have into enums, effectively making callers to spell those parameter names explicitly as C++ code.

Agreed with the above. Those comments are for code reviewers as much as people reading the code, and as someone who does a ton of code reviews, I find those VERY useful.

Though, IMO, we should just make bool-parameters against the rules and make everyone use strong types that explain exactly what they are trying to represent, but that is an obviously unpopular take

Strong -1 as well. It also makes it hard to review the code.

-1. I use IDE (qtc) with clangd backend but it doesn’t display inlay hints for argument names (at least by default), so I’d have to hover other the call to see the doc/prototype hint pop up.

MatzeB June 4, 2025, 2:51pm 9

Keep them! A function with 1 argument maybe isn’t the most convincing example (and in the OG post I would accept the code with or without comment in code review). But I find these comments invaluable in other cases. I believe there is a big readability difference between
BranchFolder BF(true, false, *MBFI, *MBPI, PSI, TailMergeSize);
and
BranchFolder BF(/*DefaultEnableTailMerge=*/true, /*CommonHoist=*/false, *MBFI, *MBPI, PSI, TailMergeSize);

Not needing to hover the mouse to see what the true/false means helps! Also as others have already mentioned there are many contexts like code-review, copy&pasting in chat, citing code in blogposts or mailing list posts, … where hovering the mouse won’t do anything.

Even better you can use the bugprone-argument-comments clang-tidy rule ( clang-tidy - bugprone-argument-comment — Extra Clang Tools 21.0.0git documentation ) and get a warning if you mix up the order (or someone refactors the API and misses this call site) it kinda becomes a way to retrofit named parameters into C++[1].

[1] I don’t think we have bugprone-argument-comments enable in the LLVM clang-tidy config yet or any bots that actually report clang-tidy warnings. But I would love for someone to enable this (or maybe if I find more time myself…).

Interesting! Yes, my implicit argument for stripping them (which I didn’t mention) was that they aren’t checked. It would be great to have this enabled!

philnik June 4, 2025, 3:50pm 11

I’m very much against this as well. I have disabled the hints because they are really bad for readability IMO. Having a comment is a lot better, because it highlights the cases where it’s actually interesting instead of having them everywhere. FWIW I very much agree with @erichkeane that strong types are even better. Maybe not make bool params against the rules, but I would very much encourage using strong types, especially in new code. That would make a ton of code so much easier to read.

I disagree with this proposal as it effectively assumes the mandatory usage of clangd or any kind of auto-completion tool. I think such assumption is too intrusive in terms of influencing developer’s choice on their tools and something LLVM as a project should not do.

re: banning bool parameters: there is a general problem when there are multiple parameters of the same type where they have no natural ordering or where the abstraction could be represented using the same types in multiple ways.

Improvements have been made to avoid bit/byte size confusion, but uintptr_t pairs (eww) that are either begin+end ranges or begin+size ranges have not been fully exorcised from the codebase afaik.