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

[Jonas Hahnfeld via Phabricator via Openmp-commits]

Hahnfeld added a comment.

Looks mostly good.

In https://reviews.llvm.org/D50522#1199469, @Hahnfeld wrote:

> Please also `clang-format` your changes.


I'm not sure whether you did this; especially the new functions in `interface.cpp` don't seem to follow the style I'm used to...



================
Comment at: libomptarget/src/interface.cpp:28-61
+// mutex
+std::mutex LibomptargetPrintMtx;
+
+
+////////////////////////////////////////////////////////////////////////////////
+/// support for print and assert
+
----------------
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.


Repository:
  rOMP OpenMP

https://reviews.llvm.org/D50522