[Openmp-commits] [PATCH] D141227: [OpenMP][libomptarget] Implement memory lock/unlock API in NextGen plugins

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert added inline comments.


================
Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.h:288
+  /// Find an allocation that intersects with a specific buffer pointer.
+  const EntryTy *findIntersecting(void *Buffer) const {
+    if (Allocs.empty())
----------------
tschuett wrote:
> jdoerfert wrote:
> > tschuett wrote:
> > > I would return `std::optional<const EntryTy*>` to avoid using `nullptr` to model failure.
> > Why would we want that? It seems to me null is a fine, didn't find the entry response. We don't need to box things just to have boxed them. 
> Because these are C idoms in C++ code. LLVM is inconsistent whether returning true or false denotes success. The boxing overhead is negligible. The intent of your APIs is much clearer. `tryFindIntersecting` is a fallible API. òptional` is a great tool to for fallible APIs. 
- Pointers/nullptr is arguably a C++ concept, we don't need to box things because C++ supports objects.
- I didn't argue about overheads, no need to start that discussion.
- There is no true or false return here so no confusion, it's `EntryTy *` that is nullptr or not.
- The API intent is to find an EntryTy, nullptr is arguably not an EntryTy object, hence none was found. How much clearer could it get if we would check an optional. Further, what would the set but nullptr case even mean?
- As far as I can see, our coding standards page does not include the word optional. If you want to have this as part of the canonical design, feel free to write an RFC. In the nextgen plugins we only use optional in the JIT, hence this is not an outlier.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141227