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

[Joel E. Denny via Phabricator via Openmp-commits]

jdenny marked an inline comment as done.
jdenny 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()
----------------
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.



Repository:
  rOMP OpenMP

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

https://reviews.llvm.org/D55588