[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:
> > 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.
Look, this is really just a minor nitpick and not a blocker for me. The device ID issue is what need's resolving before this can go in.


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

https://reviews.llvm.org/D76843