[PATCH] D33304: [WIP][clang-tidy] Add a new module Android and a new check for file descriptors.

Thu May 18 10:45:57 PDT 2017

aaron.ballman added a comment.

In https://reviews.llvm.org/D33304#758624, @srhines wrote:

> In https://reviews.llvm.org/D33304#758621, @joerg wrote:
>
> > I find the use of "must" at the very least inappropriate. If there was no use case for not including it, it wouldn't be an option. There is also nothing really Android-specific here beside maybe the open64 mess.
>
>
> On Android, we are requiring this flag. That is why this is part of a new category of Android-specific tidy rules. If you think this belongs more generally in a different category for tidy, can you suggest somewhere else to put it? We didn't want to impose these restrictions for platforms that might not want to be so strict. Also, as with any static analysis, there is the possibility that the original code author intended to "break" the rules, but that is what NOLINT is for.

I'm not keen on putting this in an Android module either, as it's not really Android-specific behavior. For instance, this is also part of a recommended compliant solution for CERT FIO22-C. I think this should probably be in misc, or the bugprone module that @alexfh has mentioned previously.

================
Comment at: clang-tidy/android/FileDescriptorCheck.cpp:66
+         "%0 must include O_CLOEXEC in their flags argument.")
+        << FD->getName()
+        << FixItHint::CreateReplacement(FlagsRange, ReplacementText);
----------------
This can just pass `FD` instead of `FD->getName()` -- that will also properly quote the function call.

This diagnostic doesn't tell the user what's wrong with their code, just how to silence the warning. I'm not keen on "must" in this diagnostic either. We don't usually tell the user they must do something unless they truly must. We usually waffle a bit and use phrase like "<reasons why the code is bad>; consider using O_CLOEXEC instead".

Repository:
  rL LLVM

https://reviews.llvm.org/D33304