Re: [PATCH] Improve CLANG support

On Fri, Oct 12, 2012 at 7:37 PM, Alex Rousskov
<rousskov_at_measurement-factory.com> wrote:
> On 08/01/2012 01:13 PM, Kinkie wrote:
>
>> the attached patch partly fixes CLANG support for Trunk.
>
>> - Slot slots[]; ///< slots storage
>> + Slot *slots; ///< slots storage
> ...
>> + slots=new Slot[limit];
>
> This and similar changes in trunk r12255 broke virtually all shared
> memory caching. What used to be a part of an object in shared memory
> became a pointer to a local memory object. Workers were not sharing
> cache contents after this (among other problems). A failed OpenSUSE
> testRock unit test exposed the bug.
>
> I believe I was OoO when this patch was posted in August so I could not
> review this in time. I am sure I would have caught this otherwise. Sorry.

Thanks for looking into it now.

> If there are no better ideas, I am going to undo these changes. Clang
> support is nice, but not as important as working code (and we should not
> expect Clang understand and help us with shared memory constraints anyway).

I agree that having something that works somewhere is better than
having something that compiles everywhere and works nowhere.

However, I would like you to explore different options, for two
reasons: one is that portability has helped us uncover some bugs in
the past, so we should try to keep it. The second is that, according
to a recent slashdot article, FreeBSD 10 could deprecate gcc in favor
of clang (see http://bsd.slashdot.org/story/12/05/13/1646230/freebsd-10-to-use-clang-compiler-deprecate-gcc),
and FreeBSD is a platform we very much care about.

> I will try to help with Clang support issues that will resurface, but I
> hope this will not make me responsible for them. The code is not buggy;
> it is Clang that has issues with it. If you need my help with Clang,
> please post details about the problem. For example, does the old code
> just create a defect in Clang report or blocks Clang from interpreting
> the whole source file?

This clang issue is with non-POD dynarrays.
Which can be solved in three ways, in my opinion: waiting for clang to
implement this feature, not using dynarrays, or using POD. While I
agree that reverting would be a good option to have something that
works immediately, may I suggest to try and use a POD dynarray +
typecasts to shuffle data in and out? It's a hack, but it should carry
us through until clang catches up.

-- 
    /kinkie