[PATCH] D134396: [OpenMP][libomptarget] New plugin infrastructure and new CUDA plugin

[Kevin Sala Penadés via Phabricator via llvm-commits]

kevinsala added inline comments.


================
Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/GlobalHandler.h:41
+  // holding a private copy of the name as a std::string
+  std::string Name;
+  uint32_t Size;
----------------
jhuber6 wrote:
> This should be able to be a `StringRef` as well.
Do you want to change it to `StringRef` for performance reasons (i.e., avoid the std::string's dynamic memory)?

Using `StringRef` here could be dangerous since we have no control on the string memory lifetime. This can be dangerous if a developer constructs a string on the stack, creates a `GlobalTy` variable, and passes that string as the name (e.g., as we do for the `exec_mode`). Thus, we would be adding an extra restriction on `GlobalTy`, where the user must guarantee that the string name has a longer lifetime than the `GlobalTy` object.

I would keep it as it is now, and if we see there is a performance penalty on using `std::string` here, we can change it.


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

https://reviews.llvm.org/D134396