[PATCH] D70469: [attributes] [analyzer] Add handle related attributes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/Attr.td:3445
+  let Spellings = [Clang<"acquire_handle">];
+  let Subjects = SubjectList<[Function, ParmVar], ErrorDiag>;
+  let Documentation = [AcquireHandleDocs];
----------------
xazax.hun wrote:
> aaron.ballman wrote:
> > What about function-like interfaces such as lambdas, blocks, or other callable objects that are not a `FunctionDecl`?
> Good point!
This get you partway there -- `FunctionLike` allows functions and function pointers, but I don't think it allows lambdas or blocks. You should add some test cases for all of these (including function pointers) to verify you're able to mark the entities you want and get the behavior you're after.

For instance, on function pointers and lambdas, this may impact the function *type* which is a good design question to be thinking about. What should happen with code like:
```
[[clang::acquire_handle]] void int open(const char *);
int (*fp)(const char *) = open;
```
If the attribute is a type attribute as opposed to a declaration attribute, then you can think about diagnostics in these sort of cases. However, if it is a type attribute, there is more work involved in hooking it up to the type system.

FWIW, this attribute smells like it should be a type attribute rather than a declaration attribute because I can imagine wanting to use this with function pointers and not losing the analysis capabilities (esp for things like callback functions).


================
Comment at: clang/include/clang/Basic/Attr.td:3451
+  let Spellings = [Clang<"use_handle">];
+  let Subjects = SubjectList<[ParmVar]>;
+  let Documentation = [UseHandleDocs];
----------------
One test that you should add is whether this case is properly handled:
```
void (*fp)(int whatever [[clang::uses_handle]]);
[](int whatever [[clang::uses_handle]]){};
```
e.g., when it's on a parameter declaration of function pointer type or a lambda, does everything still behave as expected?


================
Comment at: clang/include/clang/Basic/AttrDocs.td:4473
+  let Content = [{
+Handles are a way to identify resources like files, sockets, processes. They
+are more opaque than pointers and widely used in system programming. They have
----------------
sockets, processes -> sockets, and processes


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6523
+  // Warn if the parameter is definitely not an output parameter.
+  if (auto *PVD = dyn_cast<ParmVarDecl>(D)) {
+    if (PVD->getType()->isIntegerType()) {
----------------
`const auto *`


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6524
+  if (auto *PVD = dyn_cast<ParmVarDecl>(D)) {
+    if (PVD->getType()->isIntegerType()) {
+      S.Diag(AL.getLoc(), diag::err_attribute_output_parameter)
----------------
I'm skeptical of this. I think a better check is if the type is a pointer or reference -- was there a reason you didn't go with that approach?


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

https://reviews.llvm.org/D70469