[kdev-clang] tests: Adapt unit test to the recent changes regarding environments.

Wed Aug 27 17:16:30 UTC 2014

>Are you sure? I'll have to test this a bit more later, but I can't see how it
>could possibly work right now:

>include paths: A B C
>file gets parsed, only uses A
> only A in environment set
> environment gets serialized and stores a "hash" of A only

>restart kdevelop:
>include paths: A B C
>parse job compares the hash of that set to the serialized version, it differs
> file gets reparsed which is _wrong_

Here is how it works:

First of all there are 2 types of environment now: session environment
with _all_ includes/defines and file's environment with only used
include directories.

In session.reparse() we check if environments differ, if so we create
new ParseSession (and we did it before my commits), that's how it
should be.

If session environments match we reparse TU, and building DUChain for
it (ClangHelpers::buildDUChain).
Inside buildDUChain we work with file's environments only, so in your
case we compare A with A -> everything is ok.

Also I've just tested it manually and it works as I described...

If I understand you wrong, please provide a better description of how it fails!

>OK, I just looked at it and you indeed broke it - the failing unit test which
>you "adapted" was the sign for that! Instead, you changed the unit test which
>was a very bad decision of you.

Please, elaborate on it. How does it exactly breaks anything?

First:
ClangParsingEnvironment environment = sessionData->environment();

We get session environment with _all_ include directories and macros.

Then I remove  all include directories from it, and compare what's left with:
envFile->needsUpdate(&env)

Where envFile is environment file used by main.cpp (which doesn't use
any imports), so the includes() should be empty -> everything is ok.

So what does it break exactly?

>Did you create
>a massif/heaptrack log to look at where the memory is actually spent?

I have no time for it, but you can see it yourself, just open any
project with and without my patches. And see how much memory consumed,
and how many contexts created (just watch console), also note how much
time it takes to load the project with/without patches.

> If, as you say, it's in some QString related stuff then we can/should make use of
>Qt's implicit sharing functionality to reduce the memory consumption.

Arghh, You wrongly read my commit messages again. The problem is not
QString at all. The problem is there are many contexts for e.g.
QString file (or any other system file e.g. iostream e.t.c. ). And
this is all because of different environments used.

P.S. I don't understand why is it was needed to revert my commits? E.g.

commit 5215ff8f78ba19bd5a3b8264b7bbe9449532b03f
Author: Milian Wolff <mail at milianw.de>
Date:   Thu Aug 7 19:10:54 2014 +0200

    Update DUChain data when the environment has changed.

Is far from being perfect too:
1.  This commit completely broke code-completion!
2. Also with it there are now many versions of contexts for the same
file with different environments and as a result in 90% cases we get
true on context->parsingEnvironmentFile()->needsUpdate() -> the whole
thing runs so slowly!

So IMO you should have reverted it too!

--
Regards,
Sergey

On 8/27/14, Milian Wolff <mail at milianw.de> wrote:
> On Wednesday 27 August 2014 15:04:13 Milian Wolff wrote:
>> On Wednesday 27 August 2014 16:27:38 Sergey Kalinichev wrote:
>> > On 8/27/14, Milian Wolff <mail at milianw.de> wrote:
>> > > On Wednesday 27 August 2014 11:29:21 Sergey Kalinichev wrote:
>> > >> Git commit 575013c26d3684b5af125cb9007b50bb81e2edde by Sergey
>> > >> Kalinichev.
>> > >> Committed on 26/08/2014 at 17:54.
>> > >> Pushed by skalinichev into branch 'master'.
>> > >>
>> > >> Adapt unit test to the recent changes regarding environments.
>> > >
>> > > Can you explain this change please? I don't understand it.
>> > >
>> > > Also, thinking about your "memory optimization patch" a bit more, is
>> > > the
>> > > hash
>> > > still calculated properly? I.e. are you sure that a file won't get
>> > > reparsed
>> > > on
>> > > startup, when the same set of include paths are given but only a
>> > > partial
>> > > subset of these was used? I have the feeling that this might be
>> > > broken
>> > > now.
>> >
>> > Yes, I won't get updated as now I first calculate used include paths
>> > and only then add it into the environment.
>>
>> Are you sure? I'll have to test this a bit more later, but I can't see
>> how
>> it could possibly work right now:
>>
>> include paths: A B C
>> file gets parsed, only uses A
>> -> only A in environment set
>> -> environment gets serialized and stores a "hash" of A only
>>
>> restart kdevelop:
>> include paths: A B C
>> parse job compares the hash of that set to the serialized version, it
>> differs -> file gets reparsed which is _wrong_
>
> OK, I just looked at it and you indeed broke it - the failing unit test
> which
> you "adapted" was the sign for that! Instead, you changed the unit test
> which
> was a very bad decision of you.
>
> Please, pretty please - don't ever do something like that ever again in the
>
> future. This should have set off all alarm bells. You should have stopped
> right there and _at least_ sent a mail here to ask what the correct behavior
>
> would be. I wrote this test explicitly to check this functionality after
> all.
>
> I will revert the memory improvement patch for now as it breaks the
> functionality. Please rethink how we can improve the situation. Did you
> create
> a massif/heaptrack log to look at where the memory is actually spent? If, as
>
> you say, it's in some QString related stuff then we can/should make use of
> Qt's implicit sharing functionality to reduce the memory consumption.
>
> Bye
> --
> Milian Wolff
> mail at milianw.de
> http://milianw.de
>