[Openmp-commits] [PATCH] D40081: [CMake] Refactor common settings and flags

[Jonathan Peyton via Phabricator via Openmp-commits]

jlpeyton added inline comments.


================
Comment at: libomptarget/CMakeLists.txt:47-49
 # We need C++11 support.
-if(LIBOMPTARGET_HAVE_STD_CPP11_FLAG)
+if(OPENMP_HAVE_STD_CPP11_FLAG)
   
----------------
Hahnfeld wrote:
> jlpeyton wrote:
> > While we are cleaning up... I think this would be cleaner if all prerequisites were checked here and an error issued if one is not met.  Currently there seems to be only one (need -std=c++11 flag)  I don't think it would hurt to check for users trying to build on mac or windows here either and issue a similar message, because that would mean they tried to improperly override OPENMP_ENABLE_LIBOMPTARGET
> > 
> > ```
> > # Check prerequisites for building libomptarget
> > if (NOT OPENMP_HAVE_STD_CPP11_FLAG)
> >   libomptarget_error_say("Requested build of libomptarget with no -std=c++11 flag")
> > endif()
> > ```
> > 
> > You could also add OPENMP_HAVE_STD_CPP11_FLAG into the logic which determines the default ENABLE_LIBOMPTARGET inside the top-level CMakeLists.txt file.
> > 
> > If a user somehow finds them self inside the libomptarget/CMakeLists.txt file without -std=c++11 flag support.  I think its better for CMake to scream at the user during config time rather than for a status message to get lost in a sea of CMake output, because then the user builds all of LLVM and they scratch their heads trying to find libomptarget.
> I disagree: If the user explicitly tells the build system to build libomptarget, it's the user's fault.
> 
> Regarding C++11: Doesn't libomp also depend on that standard? Maybe we should just error out completely in the top-level `CMakeLists.txt`, I think LLVM requires a C++11-capable compiler anyway...
Actually we are in total agreement that the user is at fault.

But, one of the main goals of a configuration system is to check prerequisites and then error out when they aren't met.  I'm in favor of aiding the user in seeing the error of their ways (or setup).  At the *very least*, it should be a warning so that the output is pronounced in the CMake output.  It does not make sense to me to require something for the build, but not error out when you do not have the prerequisites  but at the same time request the feature (either by the default, ENABLE_LIBOMPTARGET, which currently does not check for the flag, or explicitly setting OPENMP_ENABLE_LIBOMPTARGET=On).  To me, this boils down to when OPENMP_ENABLE_LIBOMPTARGET=On for whatever reason, then it implies the user absolutely wants the library.

There should be checks for __attribute__((weak)) as well.

We can't error out in the top level because libomp is supported on Visual Studio which doesn't use the -std=c++11.


https://reviews.llvm.org/D40081