Code review comment for lp:~manchicken/kubuntu-debug-installer/kubuntu-debug-installer

Harald Sitter (apachelogger) wrote :

much better. I reran the entire test set but there are still some issues, in particular the krazy and smoke issues are blocking for me...

------------------ style ------------------

- indention in DebugFinder changes still has lines with 0 spaces, 2 spaces, tabs, 4 spaces. should all be 4 spaces
- also please note that our style guides says for/foreach/if/swtich-case have their opening curly braces on the same line as the keyword (foreach(...) {)
- numerous lines with indention but nothing after the indention

> m_aptTxn = 0;

better, but not perfect (http://finance.yahoo.com/q?s=TXN). please call it m_transaction or m_aptTransaction :P

> QString(DDEB_REPO_FILE)

constructing using QString::fromLatin1(..) is still faster.

new:
> QApt::SourceEntryList hardCodedRepoList;

if it is hardcoded, then it probably should be static and only be built/filled once.

> #include <KProgressDialog>

in DebugFinder.h this can now be a forward decl

> connect(m_progress, SIGNAL(cancelClicked()), m_aptTxn, SLOT(cancel()));

I personally like to line break after the signal to make it easier to spot which objects are being wired here...

connect(m_progress, SIGNAL(cancelClicked()),
        m_aptTxn, SLOT(cancel()));

------------------ smoke ------------------

(see previous smoke results
- Authentication still can not be cancelled
- ddebs entries are still added ad infinitum
- debug output still too excessive with percent values being debugged :P
- ddebs.list is still not removed at exit() if it was initially created by debug-installer

new:
- Crash when cancelling adding of sources and package lookup (http://paste.kde.org/p256c29ae/)

------------------ clang-analyzer ------------------

still happy xD

------------------ krazy2 ------------------

still issues:

krazy2 Analysis

Checkers Run = 39
Files Processed = 13
Total Issues = 9 ...as of July 30 2013 13:25:14 CEST

== messages: For File Type messages ==
1. Check for appending to rc.cpp [rcappend]... Ok!

== cmake: For File Type cmake ==
1. Check for an acceptable copyright [copyright]... Ok!

2. Check that file ends with a newline [endswithnewline]... Ok!

3. Check for an acceptable license [license]... Ok!

4. Check for spelling errors [spelling]... Ok!

== c++: For File Type c++ ==
1. Check for TRUE and FALSE macros [captruefalse]... Ok!

2. Check for methods that return 'const' refs in public classes [constref]... Ok!

3. Check for an acceptable copyright [copyright]... Ok!

4. Check for cpp macros and usage [cpp]... Ok!

5. Check for code that should be considered crashy. [crashy]... Ok!

6. Check single-char QString operations for efficiency [doublequote_chars]... Ok!

7. Check for unwanted doxygen tags in major versions [doxytags]... Ok!

8. Check public classes with private members or d-pointer issues [dpointer]... Ok!

9. Check for QString compares to "" [emptystrcompare]... Ok!

10. Check that file ends with a newline [endswithnewline]... 2 issues found
        ./src/QAptDecorator.cpp: line# 80 (1)
        ./src/LSBRelease.cpp: line# 66 (1)
        Files that do not end with a newline character can cause problems.
        Please add a newline character to the end of the file.

11. Check for C++ ctors that should be declared 'explicit' [explicit]... Ok!

12. Check for foreach loop issues [foreach]... Ok!

13. Check validity of i18n calls [i18ncheckarg]... 6 issues found
        ./src/LSBRelease.cpp: single adjective as message, probably ambiguous; explain what it refers to following the KUIT context marker line#33
        ./src/DebugFinder.cpp: invalid interface subcue :dialog to role @title line#110,186,190,196
        ./src/DebugFinder.cpp: single adjective as message, probably ambiguous; explain what it refers to following the KUIT context marker line#196
        Make the translators' job easier and detect problems in the usage
        of the i18n() calls. When the fix is not clear, check the Techbase
        article at
        <http://techbase.kde.org/Development/Tutorials/Localization/i18n_Kr
        azy> for more information.

14. Check for invalid icon names [iconnames]... Ok!

15. Check for proper include directives [includes]... 1 issue found
        ./src/QAptDecorator.cpp: include own header first line#26
        Use <..> to include installed headers; cpp file should include
        their own headers first (but below config.h); other rules apply,
        see
        <http://techbase.kde.org/Policies/Library_Code_Policy#Getting_.23in
        cludes_right>. Use include guards in headers with appropriatedly
        encoded macro names.

16. Check for inline methods in public classes [inline]... Ok!

17. Check for an acceptable license [license]... Ok!

18. Check for normalized SIGNAL and SLOT signatures [normalize]... Ok!

19. Check for assignments to QString::null [nullstrassign]... Ok!

20. Check for compares to QString::null or QString() [nullstrcompare]... Ok!

21. Check for C++ operators that should be 'const' [operators]... Ok!

22. Check for inappropriate pass-by-value function args [passbyvalue]... Ok!

23. Check for postfix usage of ++ and -- [postfixop]... Ok!

24. Check for dangerous or inefficient QByteArray usage [qbytearray]... Ok!

25. Check for Qt classes that should not be used [qclasses]... Ok!

26. Check for Qt methods that should be avoided [qmethods]... Ok!

27. Check for QMIN and QMAX macros [qminmax]... Ok!

28. Check for classes that should use the 'Q_OBJECT' macro [qobject]... Ok!

29. Check for signals: and slots: [sigsandslots]... Ok!

30. Check for spelling errors [spelling]... Ok!

31. Check for improperly initialized global static objects [staticobjects]... Ok!

32. Check for strings used improperly or should be i18n. [strings]... Ok!

33. Check for system calls to replace by KDE or Qt equivalents [syscalls]... Ok!

34. Check for typedefs that should be replaced by Qt typedefs [typedefs]... Ok!

review: Needs Fixing

« Back to merge proposal