[PATCH] D76083: [clang-tidy] Expand the list of functions in bugprone-unused-return-value

[Joe Ranieri via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGe4bb3e25e440: [clang-tidy] Expand the list of functions in 
bugprone-unused-return-value (authored by jranieri-grammatech).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76083

Files:
  clang-tools-extra/clang-tidy/bugprone/UnusedReturnValueCheck.cpp


Index: clang-tools-extra/clang-tidy/bugprone/UnusedReturnValueCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/UnusedReturnValueCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/UnusedReturnValueCheck.cpp
@@ -43,7 +43,90 @@
"::std::unique;"
"::std::unique_ptr::release;"
"::std::basic_string::empty;"
-   "::std::vector::empty")) {}
+   "::std::vector::empty;"
+   "::std::back_inserter;"
+   "::std::distance;"
+   "::std::find;"
+   "::std::find_if;"
+   "::std::inserter;"
+   "::std::lower_bound;"
+   "::std::make_pair;"
+   "::std::map::count;"
+   "::std::map::find;"
+   "::std::map::lower_bound;"
+   "::std::multimap::equal_range;"
+   "::std::multimap::upper_bound;"
+   "::std::set::count;"
+   "::std::set::find;"
+   "::std::setfill;"
+   "::std::setprecision;"
+   "::std::setw;"
+   "::std::upper_bound;"
+   "::std::vector::at;"
+   // C standard library
+   "::bsearch;"
+   "::ferror;"
+   "::feof;"
+   "::isalnum;"
+   "::isalpha;"
+   "::isblank;"
+   "::iscntrl;"
+   "::isdigit;"
+   "::isgraph;"
+   "::islower;"
+   "::isprint;"
+   "::ispunct;"
+   "::isspace;"
+   "::isupper;"
+   "::iswalnum;"
+   "::iswprint;"
+   "::iswspace;"
+   "::isxdigit;"
+   "::memchr;"
+   "::memcmp;"
+   "::strcmp;"
+   "::strcoll;"
+   "::strncmp;"
+   "::strpbrk;"
+   "::strrchr;"
+   "::strspn;"
+   "::strstr;"
+   "::wcscmp;"
+   // POSIX
+   "::access;"
+   "::bind;"
+   "::connect;"
+   "::difftime;"
+   "::dlsym;"
+   "::fnmatch;"
+   "::getaddrinfo;"
+   "::getopt;"
+   "::htonl;"
+   "::htons;"
+   "::iconv_open;"
+   "::inet_addr;"
+   "::isascii;"
+   "::isatty;"
+   "::mmap;"
+   "::newlocale;"
+   "::openat;"
+   "::pathconf;"
+   "::pthread_equal;"
+   "::pthread_getspecific;"
+   "::pthread_mutex_trylock;"
+   "::readdir;"
+   "::readlink;"
+   "::recvmsg;"
+   "::regexec;"
+   "::scandir;"
+   "::semget;"
+   

[PATCH] D76083: [clang-tidy] Expand the list of functions in bugprone-unused-return-value

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

In D76083#2026375 , 
@jranieri-grammatech wrote:

> Ping? It sounds like the consensus is to commit this as-is and, if there's a 
> negative fallout for users of clang-tidy, either split out the functions or 
> pare the list down later?


That is my understanding of the consensus position.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76083



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76083: [clang-tidy] Expand the list of functions in bugprone-unused-return-value

[Joe Ranieri via Phabricator via cfe-commits]

jranieri-grammatech added a comment.

Ping? It sounds like the consensus is to commit this as-is and, if there's a 
negative fallout for users of clang-tidy, either split out the functions or 
pare the list down later?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76083



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76083: [clang-tidy] Expand the list of functions in bugprone-unused-return-value

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tools-extra/clang-tidy/bugprone/UnusedReturnValueCheck.cpp:98
+   "::access;"
+   "::bind;"
+   "::connect;"

sammccall wrote:
> aaron.ballman wrote:
> > aaron.ballman wrote:
> > > sammccall wrote:
> > > > aaron.ballman wrote:
> > > > > sammccall wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > jranieri-grammatech wrote:
> > > > > > > > alexfh wrote:
> > > > > > > > > bind has a side effect and returns a success status. Thus, 
> > > > > > > > > the result being unused isn't necessarily a bug. Same for 
> > > > > > > > > `connect`. And probably for `setjmp` as well.
> > > > > > > > In terms of bind, connect, and setjmp: while I personally would 
> > > > > > > > say that code not using the return value is bugprone, the data 
> > > > > > > > suggests that the vast majority of developers are using these 
> > > > > > > > functions in the intended manner and the false-positive rate 
> > > > > > > > should be low.
> > > > > > > I think we have sufficient statistical data to suggest that these 
> > > > > > > APIs should be on the list because the majority of programmers 
> > > > > > > *do not* use them solely for side effects without using the 
> > > > > > > return value, so my preference is to keep them in the list.
> > > > > > I stumbled upon this review as we're considering turning this check 
> > > > > > on by default in clangd.
> > > > > > 
> > > > > > There's a significant difference between unused std::async() 
> > > > > > (programmer misunderstood contract) and unused ::connect() 
> > > > > > (ignoring error conditions). The former is ~never noise, and the 
> > > > > > latter may be (e.g. in experimental or incomplete code).
> > > > > > 
> > > > > > So there's some value in separating these two lists out either as 
> > > > > > an option or a separate named check (bugprone-unhandled-error?) I 
> > > > > > think we probably wouldn't enable this check by default if it 
> > > > > > includes the error-code functions.
> > > > > > 
> > > > > > > the majority of programmers *do not* use them solely for side 
> > > > > > > effects
> > > > > > ...in popular, distributed software :-)
> > > > > > So there's some value in separating these two lists out either as 
> > > > > > an option or a separate named check (bugprone-unhandled-error?) I 
> > > > > > think we probably wouldn't enable this check by default if it 
> > > > > > includes the error-code functions.
> > > > > 
> > > > > I think that adds complexity to this check when the complexity isn't 
> > > > > necessary. clang-tidy has traditionally been a place for checks that 
> > > > > are chattier than what the compiler should provide, and this check 
> > > > > has a trivial, well-understood mechanism to silence the diagnostics 
> > > > > (cast to void) which also expresses intent properly to the toolchain.
> > > > > 
> > > > > >>the majority of programmers *do not* use them solely for side 
> > > > > >>effects
> > > > > > ...in popular, distributed software :-)
> > > > > 
> > > > > I have not seen anyone provide data to suggest that the functions in 
> > > > > question appear in any statistically significant amount in practice 
> > > > > without checking the return value, just worries that they *could*. I 
> > > > > don't think that's compelling in the face of data. Remember, this is 
> > > > > for bugprone patterns, not bugknown patterns.
> > > > I think we're talking past each other here. I'm not saying clang-tidy 
> > > > shouldn't have the check, or that it's not a bugprone pattern, or that 
> > > > the clang-tidy default should be different.
> > > > 
> > > > But there are scenarios where you want one but not the other. 
> > > > Concretely, warnings shown in an IDE **as you type and by default**. If 
> > > > you're misusing an API rendering it completely useless, you should see 
> > > > that ASAP. If you fail to check an error code, some users won't want to 
> > > > be warned about that until later.
> > > > 
> > > > By bundling them into a single check without options (other than 
> > > > duplicating the whole list), it's hard to create that useful but 
> > > > inoffensive default setup. That's OK, clangd can remove the check from 
> > > > the whitelist, but I think we'd get a lot of value out of it.
> > > > 
> > > > > I have not seen anyone provide data to suggest that the functions in 
> > > > > question appear in any statistically significant amount in practice
> > > > Right, we don't have data either way on incomplete code. Based on 
> > > > experience of writing code and watching others write code, I believe 
> > > > people write sloppy code they'd never check in, and appreciate being 
> > > > told early when it doesn't do what they intend, but some don't 
> > > > appreciate being told to be less sloppy. Is your intuition different? 
> > > > Do you think the data 

[PATCH] D76083: [clang-tidy] Expand the list of functions in bugprone-unused-return-value

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clang-tidy/bugprone/UnusedReturnValueCheck.cpp:98
+   "::access;"
+   "::bind;"
+   "::connect;"

aaron.ballman wrote:
> aaron.ballman wrote:
> > sammccall wrote:
> > > aaron.ballman wrote:
> > > > sammccall wrote:
> > > > > aaron.ballman wrote:
> > > > > > jranieri-grammatech wrote:
> > > > > > > alexfh wrote:
> > > > > > > > bind has a side effect and returns a success status. Thus, the 
> > > > > > > > result being unused isn't necessarily a bug. Same for 
> > > > > > > > `connect`. And probably for `setjmp` as well.
> > > > > > > In terms of bind, connect, and setjmp: while I personally would 
> > > > > > > say that code not using the return value is bugprone, the data 
> > > > > > > suggests that the vast majority of developers are using these 
> > > > > > > functions in the intended manner and the false-positive rate 
> > > > > > > should be low.
> > > > > > I think we have sufficient statistical data to suggest that these 
> > > > > > APIs should be on the list because the majority of programmers *do 
> > > > > > not* use them solely for side effects without using the return 
> > > > > > value, so my preference is to keep them in the list.
> > > > > I stumbled upon this review as we're considering turning this check 
> > > > > on by default in clangd.
> > > > > 
> > > > > There's a significant difference between unused std::async() 
> > > > > (programmer misunderstood contract) and unused ::connect() (ignoring 
> > > > > error conditions). The former is ~never noise, and the latter may be 
> > > > > (e.g. in experimental or incomplete code).
> > > > > 
> > > > > So there's some value in separating these two lists out either as an 
> > > > > option or a separate named check (bugprone-unhandled-error?) I think 
> > > > > we probably wouldn't enable this check by default if it includes the 
> > > > > error-code functions.
> > > > > 
> > > > > > the majority of programmers *do not* use them solely for side 
> > > > > > effects
> > > > > ...in popular, distributed software :-)
> > > > > So there's some value in separating these two lists out either as an 
> > > > > option or a separate named check (bugprone-unhandled-error?) I think 
> > > > > we probably wouldn't enable this check by default if it includes the 
> > > > > error-code functions.
> > > > 
> > > > I think that adds complexity to this check when the complexity isn't 
> > > > necessary. clang-tidy has traditionally been a place for checks that 
> > > > are chattier than what the compiler should provide, and this check has 
> > > > a trivial, well-understood mechanism to silence the diagnostics (cast 
> > > > to void) which also expresses intent properly to the toolchain.
> > > > 
> > > > >>the majority of programmers *do not* use them solely for side effects
> > > > > ...in popular, distributed software :-)
> > > > 
> > > > I have not seen anyone provide data to suggest that the functions in 
> > > > question appear in any statistically significant amount in practice 
> > > > without checking the return value, just worries that they *could*. I 
> > > > don't think that's compelling in the face of data. Remember, this is 
> > > > for bugprone patterns, not bugknown patterns.
> > > I think we're talking past each other here. I'm not saying clang-tidy 
> > > shouldn't have the check, or that it's not a bugprone pattern, or that 
> > > the clang-tidy default should be different.
> > > 
> > > But there are scenarios where you want one but not the other. Concretely, 
> > > warnings shown in an IDE **as you type and by default**. If you're 
> > > misusing an API rendering it completely useless, you should see that 
> > > ASAP. If you fail to check an error code, some users won't want to be 
> > > warned about that until later.
> > > 
> > > By bundling them into a single check without options (other than 
> > > duplicating the whole list), it's hard to create that useful but 
> > > inoffensive default setup. That's OK, clangd can remove the check from 
> > > the whitelist, but I think we'd get a lot of value out of it.
> > > 
> > > > I have not seen anyone provide data to suggest that the functions in 
> > > > question appear in any statistically significant amount in practice
> > > Right, we don't have data either way on incomplete code. Based on 
> > > experience of writing code and watching others write code, I believe 
> > > people write sloppy code they'd never check in, and appreciate being told 
> > > early when it doesn't do what they intend, but some don't appreciate 
> > > being told to be less sloppy. Is your intuition different? Do you think 
> > > the data provided addresses this question?
> > > But there are scenarios where you want one but not the other. Concretely, 
> > > warnings shown in an IDE as you type and by default. If you're misusing 

[PATCH] D76083: [clang-tidy] Expand the list of functions in bugprone-unused-return-value

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tools-extra/clang-tidy/bugprone/UnusedReturnValueCheck.cpp:98
+   "::access;"
+   "::bind;"
+   "::connect;"

aaron.ballman wrote:
> sammccall wrote:
> > aaron.ballman wrote:
> > > sammccall wrote:
> > > > aaron.ballman wrote:
> > > > > jranieri-grammatech wrote:
> > > > > > alexfh wrote:
> > > > > > > bind has a side effect and returns a success status. Thus, the 
> > > > > > > result being unused isn't necessarily a bug. Same for `connect`. 
> > > > > > > And probably for `setjmp` as well.
> > > > > > In terms of bind, connect, and setjmp: while I personally would say 
> > > > > > that code not using the return value is bugprone, the data suggests 
> > > > > > that the vast majority of developers are using these functions in 
> > > > > > the intended manner and the false-positive rate should be low.
> > > > > I think we have sufficient statistical data to suggest that these 
> > > > > APIs should be on the list because the majority of programmers *do 
> > > > > not* use them solely for side effects without using the return value, 
> > > > > so my preference is to keep them in the list.
> > > > I stumbled upon this review as we're considering turning this check on 
> > > > by default in clangd.
> > > > 
> > > > There's a significant difference between unused std::async() 
> > > > (programmer misunderstood contract) and unused ::connect() (ignoring 
> > > > error conditions). The former is ~never noise, and the latter may be 
> > > > (e.g. in experimental or incomplete code).
> > > > 
> > > > So there's some value in separating these two lists out either as an 
> > > > option or a separate named check (bugprone-unhandled-error?) I think we 
> > > > probably wouldn't enable this check by default if it includes the 
> > > > error-code functions.
> > > > 
> > > > > the majority of programmers *do not* use them solely for side effects
> > > > ...in popular, distributed software :-)
> > > > So there's some value in separating these two lists out either as an 
> > > > option or a separate named check (bugprone-unhandled-error?) I think we 
> > > > probably wouldn't enable this check by default if it includes the 
> > > > error-code functions.
> > > 
> > > I think that adds complexity to this check when the complexity isn't 
> > > necessary. clang-tidy has traditionally been a place for checks that are 
> > > chattier than what the compiler should provide, and this check has a 
> > > trivial, well-understood mechanism to silence the diagnostics (cast to 
> > > void) which also expresses intent properly to the toolchain.
> > > 
> > > >>the majority of programmers *do not* use them solely for side effects
> > > > ...in popular, distributed software :-)
> > > 
> > > I have not seen anyone provide data to suggest that the functions in 
> > > question appear in any statistically significant amount in practice 
> > > without checking the return value, just worries that they *could*. I 
> > > don't think that's compelling in the face of data. Remember, this is for 
> > > bugprone patterns, not bugknown patterns.
> > I think we're talking past each other here. I'm not saying clang-tidy 
> > shouldn't have the check, or that it's not a bugprone pattern, or that the 
> > clang-tidy default should be different.
> > 
> > But there are scenarios where you want one but not the other. Concretely, 
> > warnings shown in an IDE **as you type and by default**. If you're misusing 
> > an API rendering it completely useless, you should see that ASAP. If you 
> > fail to check an error code, some users won't want to be warned about that 
> > until later.
> > 
> > By bundling them into a single check without options (other than 
> > duplicating the whole list), it's hard to create that useful but 
> > inoffensive default setup. That's OK, clangd can remove the check from the 
> > whitelist, but I think we'd get a lot of value out of it.
> > 
> > > I have not seen anyone provide data to suggest that the functions in 
> > > question appear in any statistically significant amount in practice
> > Right, we don't have data either way on incomplete code. Based on 
> > experience of writing code and watching others write code, I believe people 
> > write sloppy code they'd never check in, and appreciate being told early 
> > when it doesn't do what they intend, but some don't appreciate being told 
> > to be less sloppy. Is your intuition different? Do you think the data 
> > provided addresses this question?
> > But there are scenarios where you want one but not the other. Concretely, 
> > warnings shown in an IDE as you type and by default. If you're misusing an 
> > API rendering it completely useless, you should see that ASAP. If you fail 
> > to check an error code, some users won't want to be warned about that until 
> > later.
> 
> You're right, we 

[PATCH] D76083: [clang-tidy] Expand the list of functions in bugprone-unused-return-value

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tools-extra/clang-tidy/bugprone/UnusedReturnValueCheck.cpp:98
+   "::access;"
+   "::bind;"
+   "::connect;"

sammccall wrote:
> aaron.ballman wrote:
> > sammccall wrote:
> > > aaron.ballman wrote:
> > > > jranieri-grammatech wrote:
> > > > > alexfh wrote:
> > > > > > bind has a side effect and returns a success status. Thus, the 
> > > > > > result being unused isn't necessarily a bug. Same for `connect`. 
> > > > > > And probably for `setjmp` as well.
> > > > > In terms of bind, connect, and setjmp: while I personally would say 
> > > > > that code not using the return value is bugprone, the data suggests 
> > > > > that the vast majority of developers are using these functions in the 
> > > > > intended manner and the false-positive rate should be low.
> > > > I think we have sufficient statistical data to suggest that these APIs 
> > > > should be on the list because the majority of programmers *do not* use 
> > > > them solely for side effects without using the return value, so my 
> > > > preference is to keep them in the list.
> > > I stumbled upon this review as we're considering turning this check on by 
> > > default in clangd.
> > > 
> > > There's a significant difference between unused std::async() (programmer 
> > > misunderstood contract) and unused ::connect() (ignoring error 
> > > conditions). The former is ~never noise, and the latter may be (e.g. in 
> > > experimental or incomplete code).
> > > 
> > > So there's some value in separating these two lists out either as an 
> > > option or a separate named check (bugprone-unhandled-error?) I think we 
> > > probably wouldn't enable this check by default if it includes the 
> > > error-code functions.
> > > 
> > > > the majority of programmers *do not* use them solely for side effects
> > > ...in popular, distributed software :-)
> > > So there's some value in separating these two lists out either as an 
> > > option or a separate named check (bugprone-unhandled-error?) I think we 
> > > probably wouldn't enable this check by default if it includes the 
> > > error-code functions.
> > 
> > I think that adds complexity to this check when the complexity isn't 
> > necessary. clang-tidy has traditionally been a place for checks that are 
> > chattier than what the compiler should provide, and this check has a 
> > trivial, well-understood mechanism to silence the diagnostics (cast to 
> > void) which also expresses intent properly to the toolchain.
> > 
> > >>the majority of programmers *do not* use them solely for side effects
> > > ...in popular, distributed software :-)
> > 
> > I have not seen anyone provide data to suggest that the functions in 
> > question appear in any statistically significant amount in practice without 
> > checking the return value, just worries that they *could*. I don't think 
> > that's compelling in the face of data. Remember, this is for bugprone 
> > patterns, not bugknown patterns.
> I think we're talking past each other here. I'm not saying clang-tidy 
> shouldn't have the check, or that it's not a bugprone pattern, or that the 
> clang-tidy default should be different.
> 
> But there are scenarios where you want one but not the other. Concretely, 
> warnings shown in an IDE **as you type and by default**. If you're misusing 
> an API rendering it completely useless, you should see that ASAP. If you fail 
> to check an error code, some users won't want to be warned about that until 
> later.
> 
> By bundling them into a single check without options (other than duplicating 
> the whole list), it's hard to create that useful but inoffensive default 
> setup. That's OK, clangd can remove the check from the whitelist, but I think 
> we'd get a lot of value out of it.
> 
> > I have not seen anyone provide data to suggest that the functions in 
> > question appear in any statistically significant amount in practice
> Right, we don't have data either way on incomplete code. Based on experience 
> of writing code and watching others write code, I believe people write sloppy 
> code they'd never check in, and appreciate being told early when it doesn't 
> do what they intend, but some don't appreciate being told to be less sloppy. 
> Is your intuition different? Do you think the data provided addresses this 
> question?
> But there are scenarios where you want one but not the other. Concretely, 
> warnings shown in an IDE as you type and by default. If you're misusing an 
> API rendering it completely useless, you should see that ASAP. If you fail to 
> check an error code, some users won't want to be warned about that until 
> later.

You're right, we were talking past one another because this was not a scenario 
I had in mind at all. Thank you for raising it!

> Is your intuition different? Do you think the data provided addresses 

[PATCH] D76083: [clang-tidy] Expand the list of functions in bugprone-unused-return-value

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clang-tidy/bugprone/UnusedReturnValueCheck.cpp:98
+   "::access;"
+   "::bind;"
+   "::connect;"

aaron.ballman wrote:
> sammccall wrote:
> > aaron.ballman wrote:
> > > jranieri-grammatech wrote:
> > > > alexfh wrote:
> > > > > bind has a side effect and returns a success status. Thus, the result 
> > > > > being unused isn't necessarily a bug. Same for `connect`. And 
> > > > > probably for `setjmp` as well.
> > > > In terms of bind, connect, and setjmp: while I personally would say 
> > > > that code not using the return value is bugprone, the data suggests 
> > > > that the vast majority of developers are using these functions in the 
> > > > intended manner and the false-positive rate should be low.
> > > I think we have sufficient statistical data to suggest that these APIs 
> > > should be on the list because the majority of programmers *do not* use 
> > > them solely for side effects without using the return value, so my 
> > > preference is to keep them in the list.
> > I stumbled upon this review as we're considering turning this check on by 
> > default in clangd.
> > 
> > There's a significant difference between unused std::async() (programmer 
> > misunderstood contract) and unused ::connect() (ignoring error conditions). 
> > The former is ~never noise, and the latter may be (e.g. in experimental or 
> > incomplete code).
> > 
> > So there's some value in separating these two lists out either as an option 
> > or a separate named check (bugprone-unhandled-error?) I think we probably 
> > wouldn't enable this check by default if it includes the error-code 
> > functions.
> > 
> > > the majority of programmers *do not* use them solely for side effects
> > ...in popular, distributed software :-)
> > So there's some value in separating these two lists out either as an option 
> > or a separate named check (bugprone-unhandled-error?) I think we probably 
> > wouldn't enable this check by default if it includes the error-code 
> > functions.
> 
> I think that adds complexity to this check when the complexity isn't 
> necessary. clang-tidy has traditionally been a place for checks that are 
> chattier than what the compiler should provide, and this check has a trivial, 
> well-understood mechanism to silence the diagnostics (cast to void) which 
> also expresses intent properly to the toolchain.
> 
> >>the majority of programmers *do not* use them solely for side effects
> > ...in popular, distributed software :-)
> 
> I have not seen anyone provide data to suggest that the functions in question 
> appear in any statistically significant amount in practice without checking 
> the return value, just worries that they *could*. I don't think that's 
> compelling in the face of data. Remember, this is for bugprone patterns, not 
> bugknown patterns.
I think we're talking past each other here. I'm not saying clang-tidy shouldn't 
have the check, or that it's not a bugprone pattern, or that the clang-tidy 
default should be different.

But there are scenarios where you want one but not the other. Concretely, 
warnings shown in an IDE **as you type and by default**. If you're misusing an 
API rendering it completely useless, you should see that ASAP. If you fail to 
check an error code, some users won't want to be warned about that until later.

By bundling them into a single check without options (other than duplicating 
the whole list), it's hard to create that useful but inoffensive default setup. 
That's OK, clangd can remove the check from the whitelist, but I think we'd get 
a lot of value out of it.

> I have not seen anyone provide data to suggest that the functions in question 
> appear in any statistically significant amount in practice
Right, we don't have data either way on incomplete code. Based on experience of 
writing code and watching others write code, I believe people write sloppy code 
they'd never check in, and appreciate being told early when it doesn't do what 
they intend, but some don't appreciate being told to be less sloppy. Is your 
intuition different? Do you think the data provided addresses this question?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76083



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76083: [clang-tidy] Expand the list of functions in bugprone-unused-return-value

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tools-extra/clang-tidy/bugprone/UnusedReturnValueCheck.cpp:98
+   "::access;"
+   "::bind;"
+   "::connect;"

sammccall wrote:
> aaron.ballman wrote:
> > jranieri-grammatech wrote:
> > > alexfh wrote:
> > > > bind has a side effect and returns a success status. Thus, the result 
> > > > being unused isn't necessarily a bug. Same for `connect`. And probably 
> > > > for `setjmp` as well.
> > > In terms of bind, connect, and setjmp: while I personally would say that 
> > > code not using the return value is bugprone, the data suggests that the 
> > > vast majority of developers are using these functions in the intended 
> > > manner and the false-positive rate should be low.
> > I think we have sufficient statistical data to suggest that these APIs 
> > should be on the list because the majority of programmers *do not* use them 
> > solely for side effects without using the return value, so my preference is 
> > to keep them in the list.
> I stumbled upon this review as we're considering turning this check on by 
> default in clangd.
> 
> There's a significant difference between unused std::async() (programmer 
> misunderstood contract) and unused ::connect() (ignoring error conditions). 
> The former is ~never noise, and the latter may be (e.g. in experimental or 
> incomplete code).
> 
> So there's some value in separating these two lists out either as an option 
> or a separate named check (bugprone-unhandled-error?) I think we probably 
> wouldn't enable this check by default if it includes the error-code functions.
> 
> > the majority of programmers *do not* use them solely for side effects
> ...in popular, distributed software :-)
> So there's some value in separating these two lists out either as an option 
> or a separate named check (bugprone-unhandled-error?) I think we probably 
> wouldn't enable this check by default if it includes the error-code functions.

I think that adds complexity to this check when the complexity isn't necessary. 
clang-tidy has traditionally been a place for checks that are chattier than 
what the compiler should provide, and this check has a trivial, well-understood 
mechanism to silence the diagnostics (cast to void) which also expresses intent 
properly to the toolchain.

>>the majority of programmers *do not* use them solely for side effects
> ...in popular, distributed software :-)

I have not seen anyone provide data to suggest that the functions in question 
appear in any statistically significant amount in practice without checking the 
return value, just worries that they *could*. I don't think that's compelling 
in the face of data. Remember, this is for bugprone patterns, not bugknown 
patterns.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76083



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76083: [clang-tidy] Expand the list of functions in bugprone-unused-return-value

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clang-tidy/bugprone/UnusedReturnValueCheck.cpp:98
+   "::access;"
+   "::bind;"
+   "::connect;"

aaron.ballman wrote:
> jranieri-grammatech wrote:
> > alexfh wrote:
> > > bind has a side effect and returns a success status. Thus, the result 
> > > being unused isn't necessarily a bug. Same for `connect`. And probably 
> > > for `setjmp` as well.
> > In terms of bind, connect, and setjmp: while I personally would say that 
> > code not using the return value is bugprone, the data suggests that the 
> > vast majority of developers are using these functions in the intended 
> > manner and the false-positive rate should be low.
> I think we have sufficient statistical data to suggest that these APIs should 
> be on the list because the majority of programmers *do not* use them solely 
> for side effects without using the return value, so my preference is to keep 
> them in the list.
I stumbled upon this review as we're considering turning this check on by 
default in clangd.

There's a significant difference between unused std::async() (programmer 
misunderstood contract) and unused ::connect() (ignoring error conditions). The 
former is ~never noise, and the latter may be (e.g. in experimental or 
incomplete code).

So there's some value in separating these two lists out either as an option or 
a separate named check (bugprone-unhandled-error?) I think we probably wouldn't 
enable this check by default if it includes the error-code functions.

> the majority of programmers *do not* use them solely for side effects
...in popular, distributed software :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76083



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76083: [clang-tidy] Expand the list of functions in bugprone-unused-return-value

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tools-extra/clang-tidy/bugprone/UnusedReturnValueCheck.cpp:98
+   "::access;"
+   "::bind;"
+   "::connect;"

jranieri-grammatech wrote:
> alexfh wrote:
> > bind has a side effect and returns a success status. Thus, the result being 
> > unused isn't necessarily a bug. Same for `connect`. And probably for 
> > `setjmp` as well.
> In terms of bind, connect, and setjmp: while I personally would say that code 
> not using the return value is bugprone, the data suggests that the vast 
> majority of developers are using these functions in the intended manner and 
> the false-positive rate should be low.
I think we have sufficient statistical data to suggest that these APIs should 
be on the list because the majority of programmers *do not* use them solely for 
side effects without using the return value, so my preference is to keep them 
in the list.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76083



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76083: [clang-tidy] Expand the list of functions in bugprone-unused-return-value

[Joe Ranieri via Phabricator via cfe-commits]

jranieri-grammatech updated this revision to Diff 250545.
jranieri-grammatech marked an inline comment as done.
jranieri-grammatech added a comment.

Removing std::move.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76083

Files:
  clang-tools-extra/clang-tidy/bugprone/UnusedReturnValueCheck.cpp


Index: clang-tools-extra/clang-tidy/bugprone/UnusedReturnValueCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/UnusedReturnValueCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/UnusedReturnValueCheck.cpp
@@ -43,7 +43,90 @@
"::std::unique;"
"::std::unique_ptr::release;"
"::std::basic_string::empty;"
-   "::std::vector::empty")) {}
+   "::std::vector::empty;"
+   "::std::back_inserter;"
+   "::std::distance;"
+   "::std::find;"
+   "::std::find_if;"
+   "::std::inserter;"
+   "::std::lower_bound;"
+   "::std::make_pair;"
+   "::std::map::count;"
+   "::std::map::find;"
+   "::std::map::lower_bound;"
+   "::std::multimap::equal_range;"
+   "::std::multimap::upper_bound;"
+   "::std::set::count;"
+   "::std::set::find;"
+   "::std::setfill;"
+   "::std::setprecision;"
+   "::std::setw;"
+   "::std::upper_bound;"
+   "::std::vector::at;"
+   // C standard library
+   "::bsearch;"
+   "::ferror;"
+   "::feof;"
+   "::isalnum;"
+   "::isalpha;"
+   "::isblank;"
+   "::iscntrl;"
+   "::isdigit;"
+   "::isgraph;"
+   "::islower;"
+   "::isprint;"
+   "::ispunct;"
+   "::isspace;"
+   "::isupper;"
+   "::iswalnum;"
+   "::iswprint;"
+   "::iswspace;"
+   "::isxdigit;"
+   "::memchr;"
+   "::memcmp;"
+   "::strcmp;"
+   "::strcoll;"
+   "::strncmp;"
+   "::strpbrk;"
+   "::strrchr;"
+   "::strspn;"
+   "::strstr;"
+   "::wcscmp;"
+   // POSIX
+   "::access;"
+   "::bind;"
+   "::connect;"
+   "::difftime;"
+   "::dlsym;"
+   "::fnmatch;"
+   "::getaddrinfo;"
+   "::getopt;"
+   "::htonl;"
+   "::htons;"
+   "::iconv_open;"
+   "::inet_addr;"
+   "::isascii;"
+   "::isatty;"
+   "::mmap;"
+   "::newlocale;"
+   "::openat;"
+   "::pathconf;"
+   "::pthread_equal;"
+   "::pthread_getspecific;"
+   "::pthread_mutex_trylock;"
+   "::readdir;"
+   "::readlink;"
+   "::recvmsg;"
+   "::regexec;"
+   "::scandir;"
+   "::semget;"
+   "::setjmp;"
+   

[PATCH] D76083: [clang-tidy] Expand the list of functions in bugprone-unused-return-value

[Joe Ranieri via Phabricator via cfe-commits]

jranieri-grammatech added inline comments.



Comment at: clang-tools-extra/clang-tidy/bugprone/UnusedReturnValueCheck.cpp:57
+   "::std::map::lower_bound;"
+   "::std::move;"
+   "::std::multimap::equal_range;"

alexfh wrote:
> This will also affect "the other std::move" 
> (https://en.cppreference.com/w/cpp/algorithm/move).
The ambiguity here is unfortunate, but I'll remove it from the list.



Comment at: clang-tools-extra/clang-tidy/bugprone/UnusedReturnValueCheck.cpp:98
+   "::access;"
+   "::bind;"
+   "::connect;"

alexfh wrote:
> bind has a side effect and returns a success status. Thus, the result being 
> unused isn't necessarily a bug. Same for `connect`. And probably for `setjmp` 
> as well.
In terms of bind, connect, and setjmp: while I personally would say that code 
not using the return value is bugprone, the data suggests that the vast 
majority of developers are using these functions in the intended manner and the 
false-positive rate should be low.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76083



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76083: [clang-tidy] Expand the list of functions in bugprone-unused-return-value

[Joe Ranieri via Phabricator via cfe-commits]

jranieri-grammatech updated this revision to Diff 250202.
jranieri-grammatech added a comment.

Adding context.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76083

Files:
  clang-tools-extra/clang-tidy/bugprone/UnusedReturnValueCheck.cpp


Index: clang-tools-extra/clang-tidy/bugprone/UnusedReturnValueCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/UnusedReturnValueCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/UnusedReturnValueCheck.cpp
@@ -43,7 +43,91 @@
"::std::unique;"
"::std::unique_ptr::release;"
"::std::basic_string::empty;"
-   "::std::vector::empty")) {}
+   "::std::vector::empty;"
+   "::std::back_inserter;"
+   "::std::distance;"
+   "::std::find;"
+   "::std::find_if;"
+   "::std::inserter;"
+   "::std::lower_bound;"
+   "::std::make_pair;"
+   "::std::map::count;"
+   "::std::map::find;"
+   "::std::map::lower_bound;"
+   "::std::move;"
+   "::std::multimap::equal_range;"
+   "::std::multimap::upper_bound;"
+   "::std::set::count;"
+   "::std::set::find;"
+   "::std::setfill;"
+   "::std::setprecision;"
+   "::std::setw;"
+   "::std::upper_bound;"
+   "::std::vector::at;"
+   // C standard library
+   "::bsearch;"
+   "::ferror;"
+   "::feof;"
+   "::isalnum;"
+   "::isalpha;"
+   "::isblank;"
+   "::iscntrl;"
+   "::isdigit;"
+   "::isgraph;"
+   "::islower;"
+   "::isprint;"
+   "::ispunct;"
+   "::isspace;"
+   "::isupper;"
+   "::iswalnum;"
+   "::iswprint;"
+   "::iswspace;"
+   "::isxdigit;"
+   "::memchr;"
+   "::memcmp;"
+   "::strcmp;"
+   "::strcoll;"
+   "::strncmp;"
+   "::strpbrk;"
+   "::strrchr;"
+   "::strspn;"
+   "::strstr;"
+   "::wcscmp;"
+   // POSIX
+   "::access;"
+   "::bind;"
+   "::connect;"
+   "::difftime;"
+   "::dlsym;"
+   "::fnmatch;"
+   "::getaddrinfo;"
+   "::getopt;"
+   "::htonl;"
+   "::htons;"
+   "::iconv_open;"
+   "::inet_addr;"
+   "::isascii;"
+   "::isatty;"
+   "::mmap;"
+   "::newlocale;"
+   "::openat;"
+   "::pathconf;"
+   "::pthread_equal;"
+   "::pthread_getspecific;"
+   "::pthread_mutex_trylock;"
+   "::readdir;"
+   "::readlink;"
+   "::recvmsg;"
+   "::regexec;"
+   "::scandir;"
+   "::semget;"
+   "::setjmp;"
+   "::shm_open;"
+ 

[PATCH] D76083: [clang-tidy] Expand the list of functions in bugprone-unused-return-value

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

Please upload diffs with full context. If you are using `git diff` to generate 
the patch, pass the flag `-U99`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76083



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76083: [clang-tidy] Expand the list of functions in bugprone-unused-return-value

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: clang-tools-extra/clang-tidy/bugprone/UnusedReturnValueCheck.cpp:57
+   "::std::map::lower_bound;"
+   "::std::move;"
+   "::std::multimap::equal_range;"

This will also affect "the other std::move" 
(https://en.cppreference.com/w/cpp/algorithm/move).



Comment at: clang-tools-extra/clang-tidy/bugprone/UnusedReturnValueCheck.cpp:98
+   "::access;"
+   "::bind;"
+   "::connect;"

bind has a side effect and returns a success status. Thus, the result being 
unused isn't necessarily a bug. Same for `connect`. And probably for `setjmp` 
as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76083



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76083: [clang-tidy] Expand the list of functions in bugprone-unused-return-value

[Joe Ranieri via Phabricator via cfe-commits]

jranieri-grammatech created this revision.
jranieri-grammatech added reviewers: aaron.ballman, alexfh, hokein, xazax.hun.
Herald added a subscriber: rnkovacs.
Herald added a project: clang.

This change adds common C, C++, and POSIX functions to the clang-tidy unused 
return value checker.

We ran our commercial static analysis tool, CodeSonar, over the source code of 
more than 6000 projects from Fedora's package manager. During this run, we 
gathered information about the return value usages of various library 
functions. To create a list of library functions that should always have their 
return value checked, we required at least 20 projects to have used the library 
function and for over 95% of those uses to have checked the return value. 
Finally, we limited the list of functions to those from the C standard library, 
the C++ standard library, and the POSIX specification, as these are the most 
likely to be of interest to clang-tidy users.

No test cases have been added as there is already a test that ensures functions 
on this list trigger correctly and a second test that ensures the functions on 
this list do not trigger if a custom list is specified.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D76083

Files:
  clang-tools-extra/clang-tidy/bugprone/UnusedReturnValueCheck.cpp


Index: clang-tools-extra/clang-tidy/bugprone/UnusedReturnValueCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/UnusedReturnValueCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/UnusedReturnValueCheck.cpp
@@ -43,7 +43,91 @@
"::std::unique;"
"::std::unique_ptr::release;"
"::std::basic_string::empty;"
-   "::std::vector::empty")) {}
+   "::std::vector::empty;"
+   "::std::back_inserter;"
+   "::std::distance;"
+   "::std::find;"
+   "::std::find_if;"
+   "::std::inserter;"
+   "::std::lower_bound;"
+   "::std::make_pair;"
+   "::std::map::count;"
+   "::std::map::find;"
+   "::std::map::lower_bound;"
+   "::std::move;"
+   "::std::multimap::equal_range;"
+   "::std::multimap::upper_bound;"
+   "::std::set::count;"
+   "::std::set::find;"
+   "::std::setfill;"
+   "::std::setprecision;"
+   "::std::setw;"
+   "::std::upper_bound;"
+   "::std::vector::at;"
+   // C standard library
+   "::bsearch;"
+   "::ferror;"
+   "::feof;"
+   "::isalnum;"
+   "::isalpha;"
+   "::isblank;"
+   "::iscntrl;"
+   "::isdigit;"
+   "::isgraph;"
+   "::islower;"
+   "::isprint;"
+   "::ispunct;"
+   "::isspace;"
+   "::isupper;"
+   "::iswalnum;"
+   "::iswprint;"
+   "::iswspace;"
+   "::isxdigit;"
+   "::memchr;"
+   "::memcmp;"
+   "::strcmp;"
+   "::strcoll;"
+   "::strncmp;"
+   "::strpbrk;"
+   "::strrchr;"
+   "::strspn;"
+   "::strstr;"
+   "::wcscmp;"
+   // POSIX
+   "::access;"
+   "::bind;"
+   "::connect;"
+   "::difftime;"
+   "::dlsym;"
+   "::fnmatch;"
+   "::getaddrinfo;"
+   "::getopt;"
+   "::htonl;"
+