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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D33304#758808, @alexfh wrote:

> In https://reviews.llvm.org/D33304#758713, @aaron.ballman wrote:
>
> > 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 AOSP has enough specific guidelines and requirements to warrant a separate module (especially, if Android folks have plans to contribute more than one check into it ;). As for this check, if the relevant requirements of CERT and Android are really identical, we could make an alias for the check in the CERT module (or vice versa). Another possibility that comes to mind is to create a new "posix" module specifically for things related to POSIX APIs (or "unix", if we want it to be slightly broader). WDYT?


If there are plans to add more checks, then yes. However, I think I'd prefer to see at least 2-3 checks in the work (or have some commitment for doing at least that many checks) before we add a module for it. I mostly worry about adding a single check and then nothing else. (No matter what module name we're talking about, btw.) I'd be fine with android, posix, or unix, depending on the nature of the checks.

>> I think this should probably be in misc, or the bugprone module that @alexfh has mentioned previously.
> 
> I'm strongly against bloating "misc" module. It's more or less the last resort, a place for checks we have found no better place for. The proposed "bugprone" module is an attempt to address this by pulling out a large part of "misc" to a place with more definite name and purpose. However, in the case of this check we seem to have enough good (and more specific) alternatives to default to "misc" or even "bugprone".

I was hesitant to suggest misc, but I was hoping to avoid adding a module with a single check under it and no commitment for further ones.


Repository:
  rL LLVM

https://reviews.llvm.org/D33304