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

[Jonas Hahnfeld via Phabricator via Openmp-commits]

Hahnfeld added inline comments.


================
Comment at: openmp/libomptarget/plugins/ve/src/rtl.cpp:298
+      }
+    } else {
+      proc_handle = veo_proc_create_static(ID, tmp_name);
----------------
simoll wrote:
> 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, "...");
> }
> 
> ```
My 2 cents: I don't think this improves readability. And to be honest, I'm not aware of any part in LLVM that does error handling like this.


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

https://reviews.llvm.org/D76843