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

[Milian Wolff]

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


generally looks like very clean code, just a few comments from my side below. Imo at least the one unit test should be corrected as it (imo) is not correct.

once that is done I'm in favor of pushing that into master to give it the additional testing it needs.

good job olivier, as always :)


languages/cpp/codecompletion/context.cpp
<http://git.reviewboard.kde.org/r/100367/#comment632>

    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



languages/cpp/stringhelpers.cpp
<http://git.reviewboard.kde.org/r/100367/#comment631>

    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



languages/cpp/stringhelpers.cpp
<http://git.reviewboard.kde.org/r/100367/#comment630>

    what is this for? why is it disabled? please add a comment



languages/cpp/tests/test_cppcodecompletion.cpp
<http://git.reviewboard.kde.org/r/100367/#comment633>

    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.



languages/cpp/tests/test_cppcodecompletion.cpp
<http://git.reviewboard.kde.org/r/100367/#comment634>

    "A" should evaluate to false? I don#t understand?


- Milian


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/29f834dc/attachment.html>