LLVM code style encourages this [1], this check can detect the missing cases
and provide an automatic fixit.
[1] https://llvm.org/docs/CodingStandards.html#comment-formatting
Details
Details
- Reviewers
sammccall
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Unit Tests
Event Timeline
Comment Actions
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.
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?
Comment Actions
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 .