[PATCH] D123088: attributes: introduce allockind attr for describing allocator fn behavior

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/docs/LangRef.rst:1592
+     * "uninitialized": Any newly-allocated memory (either a new block from 
+       a "new" function or the enlarged capacity from a "realloc" function) will
+       be uninitialized.
----------------
"new" -> "alloc" here


================
Comment at: llvm/include/llvm/IR/Attributes.h:50
+  Alloc = 1 << 0,         // Allocator function returns a new allocation
+  Realloc = 1 << 1,        // Allocator function resizes the `allocptr` argument
+  Free = 1 << 2,          // Allocator function frees the `allocptr` argument
----------------
Space before comment is off here.


================
Comment at: llvm/lib/IR/Attributes.cpp:458
+    AllocFnKind Kind = getAllocKind();
+    SmallVector<std::string> parts;
+    if ((Kind & AllocFnKind::Alloc) != AllocFnKind::Unknown)
----------------
Can use StringRef here, as strings are constant?


================
Comment at: llvm/lib/IR/Verifier.cpp:2091
+        ((K & (AllocFnKind::Alloc | AllocFnKind::Realloc |
+               AllocFnKind::Free)) == AllocFnKind::Unknown))
+      CheckFailed(
----------------
This enumeration of different combinations seems a bit awkward, maybe we can do something along these lines instead?
```
AllocFnKind Type = K & (AllocFnKind::Alloc | AllocFnKind::Realloc | AllocFnKind::Free);
if (!is_contained(Type, {AllocFnKind::Alloc, AllocFnKind::Realloc, AllocFnKind::Free))
  CheckFailed(...);
```


================
Comment at: llvm/lib/IR/Verifier.cpp:2094
+          "'allockind()' requires exactly one of alloc, realloc, and free");
+    if (((K & AllocFnKind::Free) != AllocFnKind::Unknown) &&
+        ((K & (AllocFnKind::Uninitialized | AllocFnKind::Zeroed |
----------------
The `!= AllocFnKind::Unknown` checks here aren't really necessary, and imho don't really add to readability either. `(K & AllocFnKind::Free)` should be directly usable as a boolean. (Though I don't care strongly about this.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123088