[Openmp-commits] [PATCH] D50522: [OpenMP][libomptarget] Bringing up to spec with respect to OMP_TARGET_OFFLOAD env var

[Alexandre Eichenberger via Phabricator via Openmp-commits]

AlexEichenberger marked an inline comment as done.
AlexEichenberger added inline comments.


================
Comment at: libomptarget/src/interface.cpp:28-61
+// mutex
+std::mutex LibomptargetPrintMtx;
+
+
+////////////////////////////////////////////////////////////////////////////////
+/// support for print and assert
+
----------------
Hahnfeld wrote:
> AlexEichenberger wrote:
> > Hahnfeld wrote:
> > > This seems overly complex where we now only need a single `fprintf` (which is thread-safe) inside an `if`. Do you have upcoming patches that will use these functions?
> > Yes, I intend this to be used for all output feedback to users. Ideally, I would find a way to share the infrastructure in kmpc, but for the moment this should do it. Note that the lock is only acquired prior to aborting, so overheads are not an issue.
> In that case I think the functions should be moved to `omptarget.cpp` (or a completely new file?) because they'll be used by more than just the interface?
> 
> Inverting `cond` seems weird, could you explain that choice? If that's to model `assert` I think the function should be renamed to `AssertOrFatalMassage`. To me the current function name implies `if (cond) FatalMessage(...)`, but that may be just me.
I agree.. maybe private.cpp?

I named them AssertWithFatalMessage, but then you mentioned that asserts are turned off in the release mode, so I though that would be confusing too.

But if you prefer Assert, that was my first choice too


Repository:
  rOMP OpenMP

https://reviews.llvm.org/D50522