[Openmp-commits] [PATCH] D89654: [WIP][libomptarget][NFC] Refactor Libomptarget functionality into a class

[Jon Chesterfield via Phabricator via Openmp-commits]

JonChesterfield added a comment.

Getting rid of global state is a worthy goal. I think you've found all the global state, put it in a class, moved the functions acting on it into methods, then instantiated that class once to get a single object that is passed into each function.

Grouping all the state in a class that is instantiated once and passed in is good. I think moving all the functions into members that act on that one instance is a step too far, especially in the same patch. Having the functions callable from a C API is a good feature.

You could make a class instance with public fields that is passed to each of the C functions (by pointer), put all the global state in that class, and stay with free functions. No member functions. Probably not even a constructor, as we might want to handle failing to construct. That'll be functionally equivalent to the above and a much smaller patch to review.

If the consensus is that we we want the class methods instead of free functions, that can be a follow up patch that moves free functions into the class, which will be easier to read after the above.



================
Comment at: openmp/libomptarget/src/rtl.cpp:25
 
 __attribute__((constructor(101))) void init() {
   DP("Init target library!\n");
----------------
If construction fails, we presumably call std::terminate here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89654