llvm dev - Aug 2010 - [LLVMdev] [PATCH] MSVC: Allow choosing different CRT for different build types

This patch splits LLVM_USE_CRT into new CMake variables
LLVM_USE_CRT_DEBUG, LLVM_USE_CRT_RELEASE, etc (one for each build
type). It also automatically detects which CRT was already chosen by
CMake's defaults in the CMAKE_CXX_FLAGS_* variables, and defaults to
those values.

(Before, it was using add_llvm_definitions(), which worked...somehow,
but CMAKE_CXX_FLAGS_RELEASE and friends were still defining their own
CRT flag. Now it edits the FLAGS variables instead.)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: add-build-config-crt-selection-v4.patch
Type: text/x-patch
Size: 4950 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20100802/1c8f91a1/attachment.bin>

nobled <nobled at dreamwidth.org> writes:
> This patch splits LLVM_USE_CRT into new CMake variables
> LLVM_USE_CRT_DEBUG, LLVM_USE_CRT_RELEASE, etc (one for each build
> type). It also automatically detects which CRT was already chosen by
> CMake's defaults in the CMAKE_CXX_FLAGS_* variables, and defaults to
> those values.
>
> (Before, it was using add_llvm_definitions(), which worked...somehow,
> but CMAKE_CXX_FLAGS_RELEASE and friends were still defining their own
> CRT flag. Now it edits the FLAGS variables instead.)
I'm a bit wary about this patch. So much complexity for so petty
feature... Maybe the right thing is to determine the scenarios where
people set LLVM_USE_CRT. Maybe all we need is to define another build
type that inherits from Release which uses the debug version of the CRT.

Anyways, the patch have some issues: remove commented-out code, use the
`foreach' instead of `FOREACH', as in the rest of the code (same for
other keywords), either remove the user-configurable variable
LLVM_USE_CRT or exit with an error if the user sets it, add an extensive
comment about the motivation of the code and about how it achieves its
purpose. Maybe put it on a new file under cmake/modules for stuff
specific of Visual Studio.

But first I'll like to see if we can simplify the requirements.

[snip]

Óscar Fuentes <ofv at wanadoo.es> wrote:
> I'm a bit wary about this patch. So much complexity for so petty
> feature... Maybe the right thing is to determine the scenarios where
> people set LLVM_USE_CRT. Maybe all we need is to define another build
> type that inherits from Release which uses the debug version of the CRT.
>
> Anyways, the patch have some issues: remove commented-out code, use the
> `foreach' instead of `FOREACH', as in the rest of the code (same
for
> other keywords), either remove the user-configurable variable
> LLVM_USE_CRT or exit with an error if the user sets it, add an extensive
> comment about the motivation of the code and about how it achieves its
> purpose. Maybe put it on a new file under cmake/modules for stuff
> specific of Visual Studio.
Sure; fixed-up version attached.
>
> But first I'll like to see if we can simplify the requirements.
>
> [snip]
I'm not sure it's possible to simplify it too much--we still need the
choice between dynamic and static CRT, too. Like Mesa/Gallium3d: by
default CMake has LLVM compiled with the dynamically linked CRT, but
Gallium needs a static CRT and right now it expects LLVM's Debug build
to use /MTd and Release to use /MT. If we add a bunch of new
build-types like DebugStatic, ReleaseStatic, ReleaseStaticDebug, etc
for all the use cases, it seems like it'd end up just as complicated
as having a separate option for each type.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: add-build-config-crt-selection-v8.patch
Type: text/x-patch
Size: 4889 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20100803/bb0f49e6/attachment.bin>