[Openmp-commits] [PATCH] D102230: [libomptarget][amdgpu][nfc] Expand errorcheck macros

[Jon Chesterfield via Phabricator via Openmp-commits]

JonChesterfield added inline comments.


================
Comment at: openmp/libomptarget/plugins/amdgpu/impl/data.cpp:52-54
+    printf("[%s:%d] %s failed: %s\n", __FILE__, __LINE__, "atmi_malloc",
+           get_error_string(err));
+    exit(1);
----------------
protze.joachim wrote:
> JonChesterfield wrote:
> > pdhaliwal wrote:
> > > Will it be better to use assert's here?
> > No, malloc can fail, and its failure can be handled. Assert might be better than exit, but what we actually need is return some-error-code
> Isn't that was the lines below do?
Yep. Nice example of something that is obvious once the macro is expanded, and was missed by repeated reading with the macro in use.


================
Comment at: openmp/libomptarget/plugins/amdgpu/impl/data.cpp:60
   if (err != HSA_STATUS_SUCCESS)
     ret = ATMI_STATUS_ERROR;
 
----------------
protze.joachim wrote:
> This line cannot be reached with your change.
Then it was unreachable before. Not great, kind of why I don't like control flow escaping macros.

All this change does is macroexpand ErrorCheck et al and s_else {};__g then clang format. Quite literally, I write this with regex replace / run preprocessor / regex replace.

Functional changes / stuff that requires thought intentionally left for the next round. See also D102228 for the previous incremental change which made that scripting simple.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102230