[Openmp-commits] [PATCH] D86804: [OpenMP] Consolidate error handling and debug messages in Libomptarget

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert added a comment.
Herald added a subscriber: danielkiss.

I have a lot of comments that do not directly relate to your changes but basically the code before. Anyway, while we are moving stuff we can clean it up as well.

I have no conceptual problem with this, after the nits have been addressed, and assuming no one complains, I will accept it.



================
Comment at: openmp/libomptarget/include/omptargetmessage.h:1
+//===-- omptargetmessage.h - Target independent OpenMP target RTL -- C++ --===//
+//
----------------
jhuber6 wrote:
> Is this the right location for this file? As I understand this will be visible to the rest of Clang.
I dislike the `omptarget` prefix on headers. This is in the libomptarget subfolder, so that is redundant. I'm aware the other file has such a name, that one is bad too ;)

I would suggest something like:
- `Debug.h`
- `Output.h`
- `DebugInfo.h`

but anything like this is better IMHO.


================
Comment at: openmp/libomptarget/include/omptargetmessage.h:18
+extern int DebugLevel;
+extern int InfoLevel;
+
----------------
We should add more documentation here. Also, what is "each RTL"?


================
Comment at: openmp/libomptarget/include/omptargetmessage.h:24
+
+#include <inttypes.h>
+#define DPxMOD "0x%0*" PRIxPTR
----------------
I would image the above macro is only needed for this include.
undef it after and introduce some newlines to make that clear.
If it is not only for the include, never mind.


================
Comment at: openmp/libomptarget/include/omptargetmessage.h:51
+// implementation for messages
+////////////////////////////////////////////////////////////////////////////////
+
----------------
I'd remove this comment, move the one above into the doxygen file comment at the top of the file.


================
Comment at: openmp/libomptarget/include/omptargetmessage.h:113
+
+extern int InfoLevel;
+
----------------
Already above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86804