[PATCH] D42682: [clang-tidy] Add io-functions-misused checker

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tidy/bugprone/IoFunctionsCheck.cpp:47-49
+      "consider to cast the return value of %0 from type integer to type char, "
+      "possible loss of precision if an error has occurred or the end "
+      "of file has been reached");
----------------
hgabii wrote:
> aaron.ballman wrote:
> > This diagnostic confuses me, so perhaps you can explain the situation you want to diagnose a bit further.
> > 
> > FIO34-C is about situations where `sizeof(char) == sizeof(int)` and the call returns EOF/WEOF. In that case, there's no way to distinguish between an EOF/error and a valid character. Suggesting to explicitly cast to a character type doesn't add any safety to the code -- the user needs to insert calls to `feof()` or `ferror()` instead (to make it portable, anyway) and should store the character value in a sufficiently large integer type before doing the comparison against EOF.
> > 
> > Are you trying to catch a different kind of bug?
> Yes, I want to diagnose this problem. Do you think I need to add suggestions into the warning message to use `feof()` or `ferror()`?
I think there are two different situations to think about:

1) If the target architecture has `sizeof(char) == sizeof(int)`, no amount of casting will help the user because the bit pattern for `EOF` is the same whether stored as an `int` or a `char` on that platform. There, the user really must use `feof()` and `ferror()`. This situation is theoretically even worse for the wide versions of the APIs because `sizeof(wint_t) == sizeof(wchar_t)` quite commonly, except that it would require a terrible choice in character sets for `wchar_t` by the implementation because Unicode makes guarantees about certain bit patterns not being a valid character (like 0xFFFF in UTF-16). I don't know that we have to worry about the wide-char versions at all in practice, but if you know of a platform where this is a legitimate concern, I'd love to hear about it.

2) If the target architecture does not have this identical-size-in-band-error-indicator problem, then it might make sense to diagnose the code for portability reasons or it might make sense to not diagnose at all because the user won't run into it. Might be worth making this an optional feature of the check.

Either way, I don't know that we'll be able to suggest fixes for the user in a diagnostic message of reasonable length. This might be a case where we want to tell the user about the danger and let the documentation tell them how to fix it. Maybe a diagnostic like: `"the value for %select{EOF|WEOF}0 is indistinguishable from a valid character with the same bit pattern; consider explicit checks for error or end-of-file indicators"`


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D42682