Review Request: Refactoring and bugfixing for the cpp code completion context + tests.

[Olivier Jean de Gaalon]

> On Jan. 12, 2011, 5:32 p.m., Milian Wolff wrote:
> > languages/cpp/stringhelpers.cpp, line 74
> > <http://git.reviewboard.kde.org/r/100367/diff/2/?file=6668#file6668line74>
> >
> >     maybe a set of unit tests for this function alone would be good... right now it only gets tested passively through the completion tests, right?
> >     
> >     anyways, something for the future to think about

This function doesn't cover our needs very well. findExpressionAndPrefix is basically a list of hacks to work around it. Apparently it's a holdover from kdev 3.x.
I hope to be able to replace this and findExpressionAndPrefix with something else next.
Note that I don't consider this work done with just this patch :).


> On Jan. 12, 2011, 5:32 p.m., Milian Wolff wrote:
> > languages/cpp/codecompletion/context.cpp, line 68
> > <http://git.reviewboard.kde.org/r/100367/diff/2/?file=6665#file6665line68>
> >
> >     not sure whether it's required, but I've often seen stuff like
> >     
> >     namespace {
> >     // global stuff, local to this file
> >     }
> >     
> >     should we use this here for cleanless

I didn't put that there :)
I can move it to the cpp namespace though.


> On Jan. 12, 2011, 5:32 p.m., Milian Wolff wrote:
> > languages/cpp/stringhelpers.cpp, line 113
> > <http://git.reviewboard.kde.org/r/100367/diff/2/?file=6668#file6668line113>
> >
> >     what is this for? why is it disabled? please add a comment

Oops.


> On Jan. 12, 2011, 5:32 p.m., Milian Wolff wrote:
> > languages/cpp/tests/test_cppcodecompletion.cpp, line 192
> > <http://git.reviewboard.kde.org/r/100367/diff/2/?file=6670#file6670line192>
> >
> >     I disagree, it should be valid, as the expression is "valid" for code completion, you *want* code completion after "something->". Only semantic analysis shows that "asdf" has no meaning in the given context, hence resulting in an empty list of results.
> >     
> >     the same for the stuff below imo.

But ExpressionParser is the semantic analysis, See testContextValidity().
You do get code completion after testing "something->", but only if "something" is declared valid by the ExpressionParser.
I'm quite sure that the undeclared "asdf" should not be considered valid by the expression parser.


> On Jan. 12, 2011, 5:32 p.m., Milian Wolff wrote:
> > languages/cpp/tests/test_cppcodecompletion.cpp, line 211
> > <http://git.reviewboard.kde.org/r/100367/diff/2/?file=6670#file6670line211>
> >
> >     "A" should evaluate to false? I don#t understand?

See testContextValidity in my patch, as well as line 560 in the current revision.
Basically since you don't declare a namespace, the ExpressionParser can't give you valid declarations for it. However, it should still remain valid of course.


- Olivier Jean de


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/100367/#review851
-----------------------------------------------------------


On Jan. 12, 2011, 3:51 a.m., Olivier Jean de Gaalon wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100367/
> -----------------------------------------------------------
> 
> (Updated Jan. 12, 2011, 3:51 a.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Summary
> -------
> 
> Refactoring and bugfixing for the cpp code completion context + tests.
> 
> 
> Diffs
> -----
> 
>   languages/cpp/codecompletion/context.h dd7416f 
>   languages/cpp/codecompletion/context.cpp 79e0bf3 
>   languages/cpp/codecompletion/helpers.cpp 125739a 
>   languages/cpp/codecompletion/item.cpp 5fea5fd 
>   languages/cpp/stringhelpers.cpp ace90e6 
>   languages/cpp/tests/test_cppcodecompletion.h f2e50b7 
>   languages/cpp/tests/test_cppcodecompletion.cpp 8218346 
> 
> Diff: http://git.reviewboard.kde.org/r/100367/diff
> 
> 
> Testing
> -------
> 
> About a week of use and writing tests.
> 
> 
> Thanks,
> 
> Olivier Jean de
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20110112/572ba0f8/attachment.html>