[Openmp-commits] [PATCH] D55588: [OpenMP] Fix nvidia-cuda-toolkit detection on Debian/Ubuntu

Thu Jan 3 17:44:13 PST 2019

grokos added inline comments.

================
Comment at: openmp/libomptarget/cmake/Modules/LibomptargetGetDependencies.cmake:116
+if (CUDA_TOOLKIT_ROOT_DIR)
+  set(cuda_toolkit_root_dir_old TRUE)
+endif()
----------------
jdenny wrote:
> grokos wrote:
> > jdenny wrote:
> > > grokos wrote:
> > > > There's a convention that cmake variables are capitalized. Also, cmake variables local to libomptarget should start with the prefix `LIBOMPTARGET_`. So this variable's name should be changed to `LIBOMPTARGET_CUDA_TOOLKIT_ROOT_DIR_PRESET` ("preset" makes more sense than "old").
> > > Thanks for the review.  Sure, I'll make those changes.
> > > 
> > > However, I had thought that non-cached or local variables are usually lowercase.  Is there a distinction here I'm not seeing, or is that just a different style than you prefer here?
> > True, local variables are lowercase, but non-cached variables are still capitalized. For `LIBOMPTARGET_CUDA_TOOLKIT_ROOT_DIR_PRESET` and `LIBOMPTARGET_CUDA_LIBDEVICE_SUBDIR` I prefer the capitalized version because these variables may be used outside this file in the future (although right now they are only used locally).
> So your decision to capitalize in this case is based on your expectations about how usage could evolve in the future.   Conversely, the `candidate_dir` from the other comment could be considered non-local, but your expectation is that it won't ever be used that way.  Does all that sound right?  That's all fine.  I just want to be sure I understand.  Thanks.
> 
Yes, that's pretty much it :)

Repository:
  rOMP OpenMP

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

https://reviews.llvm.org/D55588