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

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

jdenny marked 8 inline comments 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:
> > > 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 :)
Thanks, and thanks for the review!


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55588