[cmake-developers] [PATCH] LoadCommand test: cleanup

[Rolf Eike Beer]

> On Mon, Oct 17, 2011 at 9:18 AM, Rolf Eike Beer <eike at sf-mail.de> wrote:
>>> On 10/6/2011 8:24 AM, Rolf Eike Beer wrote:
>>>
>>>> Bill, that Watcom stuff was introduced by you in
>>>> 9891260a6dab66c9ea44b5c2e244f3128625baf5.
>>>> I simply assumed it was a debug leftover as setting a variable to the
>>>> value it already has looks pretty pointless to me.
>>>>
>>>> diff --git a/Tests/LoadCommand/CMakeCommands/CMakeLists.txt
>>>> b/Tests/LoadCommand/CMakeCommands/CMakeLists.txt
>>>> index 953d05c..5cdbc59 100644
>>>> --- a/Tests/LoadCommand/CMakeCommands/CMakeLists.txt
>>>> +++ b/Tests/LoadCommand/CMakeCommands/CMakeLists.txt
>>>> @@ -5,9 +5,6 @@ IF (MUDSLIDE_TYPE MATCHES MUCHO)
>>>>     ADD_DEFINITIONS(-DMUCHO_MUDSLIDE)
>>>>   ENDIF (MUDSLIDE_TYPE MATCHES MUCHO)
>>>>
>>>> -IF(WATCOM)
>>>> -  SET(CMAKE_C_FLAGS "${CMAKE_C_FLAGS}")
>>>> -ENDIF(WATCOM)
>>>
>>> Hmmm...  It could be some sort of odd escape thing going on that an
>>> extra pass by the CMake parser fixes...  I guess we can push it into
>>> the
>>> dashboard and see if it fails.  I can not remember at this point, but
>>> watcom is a picky odd compiler.
>>
>> Is anyone going to pick this patch up?

> Why is this patch important?
>
> Sometimes, some of the "useless" checking that we do in the CMake test
> suite is to make sure there's better code coverage on the dashboard,
> even if the results of checks are not used. I don't think that's the
> case here... but there are sometime non-obvious reasons behind some of
> the lines in our tests.
>
> Since this is just in the test suite, I see no reason to rush it into
> a release at the last minute.
>
> (But if it's important to you, I'm willing to be convinced that it
> should be in the release... But you'll have to convince me.)

Well, I don't think it's that important that you should do an extra test
or whatever for the next release. It's just that this code, well, looks
totally wrong. And I would like for this to eventually go upstream. No
problem if you put it in some branch and wait for it until the development
for 2.8.8 opens.

Otherwise this code just looks wrong (tm). If this actually changes
anything this looks like a real bug we should find better sooner than
later, no? ;)

About the coverage: I thought about this, but putting stuff in some
totally unrelated tests just doesn't feel right at all. We should try to
get some coverage e.g. for the CheckSymbolExists testcase that could prove
why this is broken for some gcc optimization settings so that the
FindThreads patch I sent didn't work. So if coverage is a problem this
should go into it's own test that clearly states what it is testing and
why and not being hidden in some other random test.

Greetings,

Eike