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

[Milian Wolff]

On Wednesday 27 August 2014 21:16:30 Sergey Kalinichev wrote:
> >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!

You manually had to change the unit test to prevent the bug from happening. It 
explicitly tested the case I'm describing:

- we parse a file with a set of include paths
- none of them are used, all get discarded
- this minimized environment is set in the ClangParsingEnvironmentFile via 
createTopContext in clanghelpers.cpp
- the env file stores this environment as a hash internally

now we restart kdevelop, or open an included file or anything
- the parsejob again has the same set of include paths as above
- we check isUpdateRequired which iterates over the environment files and 
calls needsUpdate
NOTE: Reading the code, I seem to have made an error here as well when I 
implemented the clang code - I forgot to commit a part of my change :) Fixed 
that now.
now it calls needsUpdate with the environment set _in the parse job_, which 
includes the full set of include files
- this hash-compared to the serialized environment which differs
- we needlessly reparse the file
- your check only comes into effect later, after we parsed the file in clang, 
which then just skips the duchain building but not the costly clang reparsing 
which we want to avoid

> >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?

see above.

> 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)

This is the error, you must not remove anything. See above, otherwise the 
check in needsUpdate will break

> 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?

See above, do you see the error now?

> >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.

OK, I'll investigate this.

> > 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.

OK, thanks for the clarification (and see below).

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

See below.

> 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!

It did? I was not aware of that. I forgot to add some files and Kevin did 
reverted that commit, which was fine. I added the files and reverted the 
revert. But how did this break code-completion? Due to the many contexts, I 
assume? I'll check this and will add a unit test for this breakage.

> 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!

Yes, this sounds like a bug indeed, and one which apparently neither me nor 
Kevin were aware of. Please notify us about such breakages in the future. And 
I agree, this sounds like a big issue and I'm perfectly fine with reverting it 
_again_ and work on it to ensure this does not happen again.

Generally, I'd rather we revert broken commits than leaving the plugin in a 
broken state. If the fix is simple or one has the time, then it's fine with me 
if someone tries to fixup commits rather than reverting them. If it takes too 
long though, reverting is just fine. It's easy to revert a revert, too.

Also, most of this stuff wouldn't happen if I could talk to you on IRC. 
Really, pretty please, could we convince you to idle in #kdevelop on freenode? 
Then I could simply discuss this with you directly instead of reverting stuff 
and writing mails. Similar, if these patches would have been on reviewboard we 
could have discussed this beforehand instead of post-hoc.

Bye

And: I'm sorry if this annoys you or if I'm stepping on your toes. I'm just 
trying my best in ensuring the quality of the kdev-clang plugin stays high and 
we do not add too many complicated workarounds and the like. I'm really very 
glad about your contributions - I'd just welcome if you could work better with 
the community. Tell us about what you are working on and the issues you find. 
Post patches on review board. Talk to us on IRC. All of that should ensure 
this issue won't repeat itself.

-- 
Milian Wolff
mail at milianw.de
http://milianw.de