[Openmp-commits] [PATCH] D76843: [Openmp] Libomptarget plugin for NEC SX-Aurora

[Simon Moll via Phabricator via Openmp-commits]

simoll added inline comments.


================
Comment at: openmp/libomptarget/plugins/ve/src/rtl.cpp:298
+      }
+    } else {
+      proc_handle = veo_proc_create_static(ID, tmp_name);
----------------
Hahnfeld wrote:
> simoll wrote:
> > manorom wrote:
> > > simoll wrote:
> > > > else after return
> > > I don't think so: If the process handle for a dynamic process is created successfully, we do not return but also do not want to visit the else branch.
> > This is simply a matter of coding style. You can rewrite the `else` to
> > 
> > ```
> > if (!is_dyn) {
> > }
> > ```
> > 
> > and it is clear under which condition it will be executed.
> This would repeat the condition on `is_dyn` which is less desirable IMHO. I agree with @manorom here: The else is not on the branch with the `return`, which is handling a possible error (ie `!proc_handle`). The code is properly indented, so at least to me it's obvious what's happening here:
> 
> ```lang=c
> if (...) {
>   // veo_proc_create
>   if (error) {
>     // log
>     return NULL;
>   }
>   // success
> } else {
>   // veo_proc_create_static
>   if (error) {
>     // log
>     return NULL;
>   }
> }
It's not a major concern in any case.
However, you have this error handler pattern repeated a lot in your code (7 times in this function):
```
if (error condition) {
  DP(<pretty print some error message>)
  return NULL
}
```
Here is an idea. You could wrap that in a macro to the reduce the amount of repetition and indentation in the code.
```
if (is_dyn) {
  proc_handle = veo_..
  RETURN_NULL_IF(!proc_handle, "veo_proc_create() failed for device %d\n", ID)
} else { 
  proc_handle = veo_..
  RETURN_NULL_IF(!proc_handle, "...");
}

```


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

https://reviews.llvm.org/D76843