[PATCH] D78546: Enable bugprone-argument-comments check in llvm.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

In D78546#2005306 <https://reviews.llvm.org/D78546#2005306>, @sammccall wrote:

> > LLVM code style encourages this
>
> Not very strongly, it just says it can be helpful (and tells you how to do it if you do it). So I'm wary of making it mandatory in cases where it isn't a clear readability win.


agree, my attemption was not to make it mandatory. a clang-tidy check is suboptimal, maybe a clangd tweak is a better choice.

every time I write a comment like this manually, I wish if there is a tool that could help me doing this stuff automatically, and then I find this clang-tidy check.

> Had a look at this on clangd code:
> 
> - the basic no-options functionality (wrong name for comment) looks good - some of them don't matter much, but didn't find any where the fix made it worse
> - the single-argument cases were almost all false positives, that flag should be flipped
>   - but it seems to count parameters rather than arguments so `foo.substr(1)` still fires
> - this is no good for standard library functions, because (at least with libstdc++):
>   - there are leading underscores in the names, these are ignored for matching but included in suggestions
>   - the standard doesn't really define the names (at least implementations don't match cppreference) so the check results aren't portable
>   - e.g. it fired on basic_string(/*Repeat=*/..., 'x') and wanted Repeat replaced with `__n`.
> - things of the form `range(0, foo.size())` => `range(/*Start=*/0, foo.size())` seem unhelpful
> 
>   I think this would be a better check if we **increased the arg threshold to 3** and **excluded functions in namespace std**. As it is I'm not sure whether it does more good than harm. WDYT?

sounds good to me (we also receive some internal bugs about the false positives on std symbols), that needs to extend the check to support these options .


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78546/new/

https://reviews.llvm.org/D78546