Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Code cleanup and performance improvements - Part 2 #2443

Merged
merged 6 commits into from Nov 29, 2018

Conversation

Projects
None yet
3 participants
@brainplot
Copy link
Contributor

brainplot commented Oct 31, 2018

I found a few more issues after a more thorough inspection of Clazy, that was disabled by default.

Description

  • Avoid allocating temporary containers
    Calling toLower() on a QString will cause a new string to be allocated. Since the function is essentially invoked to perform a case-insensitive comparison, we can invoke the contains() method with the Qt::CaseInsensitive enum value to do just that, as opposed to calling toLower() on two different strings.

  • Remove unused variables
    For some reason there were completely unused variables in the code. One of them was even initialized with the result of a method, but never used nonetheless (maybe it was the source of a bug).

  • Use QVariant::toUrl() instead of QVariant::value<QUrl>()
    The analyzer was suggesting me to do this. I guess the latter is more performant since it's not a template πŸ€”

  • Fix typo in parameter name
    This is a very minor thing. There was a typo in the declaration of a function (erroMessage instead of errorMessage), which was spelt correctly in the definition of the same function, and the analyzer reported the inconsistency between the two.

  • Improve type-safety
    Some zeros used to initialize a pointer were left out of my previous commit. I think they were introduced with the PR which adds more expiration presets.

How has this been tested?

Compiled with all tests enabled (GUI tests included) and -DWITH_DEV_BUILD on and all tests passed.

Types of changes

  • βœ… Bug fix (non-breaking change which fixes an issue)

Checklist:

  • βœ… I have read the CONTRIBUTING document. [REQUIRED]
  • βœ… My code follows the code style of this project. [REQUIRED]
  • βœ… All new and existing tests passed. [REQUIRED]
  • βœ… I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]

@brainplot brainplot force-pushed the brainplot:code-cleanup branch from d05842a to 0877a3e Oct 31, 2018

@phoerious

This comment has been minimized.

Copy link
Member

phoerious commented Oct 31, 2018

Please revert the for loop changes. Qt containers will not detach as long as you don't call non-const functions on them or the reference counter is 1.

@phoerious
Copy link
Member

phoerious left a comment

See comment.

@brainplot

This comment has been minimized.

Copy link
Contributor Author

brainplot commented Oct 31, 2018

@phoerious I just read about this feature which I didn't know about. However, isn't it better to explicitly state in code the intent of not copying a container instead of relying on an implementation detail?
This is just my thought on this. If it's wrong, I can revert my commit, no problem.
Even on StackOverflow, people seem to agree that keys() is not an efficient solution for iterating over a QMap.

@phoerious

This comment has been minimized.

Copy link
Member

phoerious commented Oct 31, 2018

We use asConst() whenever we are iterating over a non-const container and there is no reason for copying rvalues.
Implicit sharing is not an implementation detail but an integral part of the Qt API.

@brainplot

This comment has been minimized.

Copy link
Contributor Author

brainplot commented Oct 31, 2018

Ok, I'll revert the changes to the for-loops in my commit.
Do I have to revert my change to the toLower() call? Using Qt:CaseInsensitive will avoid the allocation of the new string and seems a more natural approach to the problem to me.

@droidmonkey
Copy link
Member

droidmonkey left a comment

I am not a fan of using iterators in Qt, it kind of defeats the whole point.

Show resolved Hide resolved src/core/EntryAttachments.cpp Outdated
Show resolved Hide resolved src/sshagent/SSHAgent.cpp Outdated
@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Oct 31, 2018

Keep the Qt:CaseInsensitive, that is a good change and makes the code more readable in general.

@phoerious

This comment has been minimized.

Copy link
Member

phoerious commented Oct 31, 2018

I haven't reviewed the full code yet. But reverting the iterator changes is the most important point. You may use STL-style iterators for maps for efficiency, but it's extremely ugly. After all, we don't really have performance problems.

@brainplot brainplot force-pushed the brainplot:code-cleanup branch from 0877a3e to 2c5652b Oct 31, 2018

@brainplot

This comment has been minimized.

Copy link
Contributor Author

brainplot commented Oct 31, 2018

@phoerious @droidmonkey I reverted my changes.

@brainplot

This comment has been minimized.

Copy link
Contributor Author

brainplot commented Nov 1, 2018

I ran make format and noticed that the beautifier applied a lot of fixes to the code formatting.
I've gone through each file one by one with my difftool, and wrapped with // clang-format off and // clang-format on those lines that looked more readable with manual formatting rather than automatic formatting.

I did this because, when I made my first PR, I wanted to make sure that my code followed the project's coding style, as stated in the CONTRIBUTING document; when I ran make format, however, I ended up with lots of modified files in my working directory. Now people should be able to format their code automatically.

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Nov 1, 2018

Can I give like 5 hearts? Edit, 1 per comment 😁

@brainplot brainplot force-pushed the brainplot:code-cleanup branch 2 times, most recently from 2de1d57 to 13e1b01 Nov 1, 2018

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Nov 2, 2018

Can you add these changes to your list:

Function
    isNull() const : bool
Found usages  (7 usages found)
    DatabaseOpenWidget.cpp  (1 usage found)
        169 if (!pw.isNull()) {
    DatabaseTabWidget.cpp  (2 usages found)
        160 if (!i.value().dbWidget->dbHasKey() && !(pw.isNull() && keyFile.isEmpty())) {
        199 if (!pw.isNull() || !keyFile.isEmpty()) {
    KdbxXmlReader.cpp  (1 usage found)
        734 if (!ref.first.isNull() && !ref.second.isNull()) {
    TestKeePass1Reader.cpp  (1 usage found)
        277 if (!password.isNull()) {
    TestKeys.cpp  (2 usages found)
        229 if (!error.isNull()) {
        287 if (!error.isNull()) {

QString::isNull() is inappropriate usage, should be QString::isEmpty()

@brainplot

This comment has been minimized.

Copy link
Contributor Author

brainplot commented Nov 2, 2018

@droidmonkey Done! I also replaced a few checks of the form of if (obj.size() > 0) with if (!obj.empty()).
It looks more readable in my opinion.

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Nov 10, 2018

Please rebase onto the new develop. @phoerious please update your review.

@brainplot brainplot force-pushed the brainplot:code-cleanup branch from 93087a0 to a86cf50 Nov 10, 2018

@brainplot

This comment has been minimized.

Copy link
Contributor Author

brainplot commented Nov 10, 2018

@droidmonkey done!

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Nov 17, 2018

Going to merge in the advanced search PR, then Phoerious' upcoming databasetabwidget refactor, then we'll rebase this one and merge next.

The changes were made

@droidmonkey droidmonkey added this to the v2.4.0 milestone Nov 17, 2018

@brainplot

This comment has been minimized.

Copy link
Contributor Author

brainplot commented Nov 17, 2018

Good, let me know when you're done so that I can rebase.

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Nov 24, 2018

@brainplot major refactors are complete, please rebase and fixup where necessary.

@brainplot brainplot force-pushed the brainplot:code-cleanup branch 2 times, most recently from 2f778b0 to ed9ba73 Nov 25, 2018

@brainplot brainplot force-pushed the brainplot:code-cleanup branch 4 times, most recently from 57375cf to 1eeb133 Nov 25, 2018

@brainplot

This comment has been minimized.

Copy link
Contributor Author

brainplot commented Nov 25, 2018

Alright, this one was a little tricky to rebase because there was a good bunch of conflicts to deal with; but I managed it to do it correctly, I believe, since I ran all the tests and they all passed.

I made one more change at the end: there were two variables that could have their scope reduced by making them have static storage.
In one case, the variable was declared in the .cpp file as file-scoped but it was only used in a single function. I moved the variable into the function it was used in and made it static so that it'll get initialized only once regardless of how many times the function is called.

-// Escape common regex symbols except for *, ?, and |
-auto regexEscape = QRegularExpression(R"re(([-[\]{}()+.,\\\/^$#]))re");
-
 QRegularExpression convertToRegex(const QString& string, bool useWildcards, bool exactMatch, bool caseSensitive)
 {
     QString pattern = string;

+    // Escape common regex symbols except for *, ?, and |
+    static auto regexEscape = QRegularExpression(R"re(([-[\]{}()+.,\\\/^$#]))re");
+
     // Wildcard support (*, ?, |)
     if (useWildcards) {
         pattern.replace(regexEscape, "\\\\1");
         pattern.replace("*", ".*");
         pattern.replace("?", ".");
     }

     // Exact modifier
     if (exactMatch) {
         pattern = "^" + pattern + "$";
     }

     auto regex = QRegularExpression(pattern);
     if (!caseSensitive) {
         regex.setPatternOptions(QRegularExpression::CaseInsensitiveOption);
     }

     return regex;
 }

In the other case, the variable was declared as file-scoped and used in two different functions so I couldn't move it anywhere. However, enclosing it in an anonymous namespace will prevent it to "leak" into other translation units.

+namespace
+{
+    MainWindow* g_MainWindow = nullptr;
+}
+
 const QString MainWindow::BaseWindowTitle = "KeePassXC";
 
-MainWindow* g_MainWindow = nullptr;
 MainWindow* getMainWindow()
 {
     return g_MainWindow;
 }
 
 MainWindow::MainWindow()
     : m_ui(new Ui::MainWindow())
     , m_trayIcon(nullptr)
     , m_appExitCalled(false)
     , m_appExiting(false)
 {
     g_MainWindow = this;
 
     m_ui->setupUi(this);
 
    /* ... */
}
@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Nov 25, 2018

Yah that's what I meant to do πŸ˜‰

@phoerious

This comment has been minimized.

Copy link
Member

phoerious commented Nov 26, 2018

You could also use an anonymous namespace for the latter case. Perhaps a bit more idiomatic C++.

@brainplot

This comment has been minimized.

Copy link
Contributor Author

brainplot commented Nov 26, 2018

@phoerious thanks for your suggestion. I'll amend my commit in a few hours.

@brainplot brainplot force-pushed the brainplot:code-cleanup branch from 1eeb133 to dc8b04e Nov 26, 2018

brainplot added some commits Oct 31, 2018

Fix typo in parameter name
A typo in a parameter name caused an inconsistency between declaration
and definition of a function.
Restore correct formatting
Many lines were not conformant with the project's formatting rules.
This patch should fix all formatting and whitespace issues in the code
base.
A clang-format directive was put around the connect() calls containing
SIGNALs and SLOTs whose signatures would be denormalized because of the
formatting rules.
@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Nov 28, 2018

I did another rebase. I greatly simplified the use of clang-format off/on around signal/slot connections. Instead of isolating problem ones, I just wrapped the whole block with them. I fixed other areas where the clang-format comment was causing readability to go down.

@brainplot

This comment has been minimized.

Copy link
Contributor Author

brainplot commented Nov 28, 2018

I thought about doing that as well, but since turning clang-format off on a line basis will prevent it from fixing formatting issues that may be present on the whole line, I just wanted to turn it off in the narrowest scope possible. I was even considering something like this:

connect(selectionModel(),
	// clang-format off
	SIGNAL(selectionChanged(QItemSelection,QItemSelection)),
	// clang-format on
	SLOT(emitEntrySelectionChanged())
	);

but arguably that looks even uglier.

Regarding to your changes, how do I integrate them into mine so that I can then rebase my branch?

@droidmonkey droidmonkey force-pushed the brainplot:code-cleanup branch from dc8b04e to e818415 Nov 29, 2018

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Nov 29, 2018

Sorry I was waiting for my local tests to finish building, its pushed now (to your branch). The idea for me was that if we are imposing clang-format off then that means it is manually "beautiful". In those blocks we can handle the formatting.

@droidmonkey droidmonkey merged commit c66f293 into keepassxreboot:develop Nov 29, 2018

4 checks passed

CodeFactor No issues found.
Details
MacOS (KeepassXC) TeamCity build finished
Details
Ubuntu Linux (KeepassXC) TeamCity build finished
Details
Windows 10 (KeepassXC) TeamCity build finished
Details

@brainplot brainplot deleted the brainplot:code-cleanup branch Nov 29, 2018

droidmonkey added a commit that referenced this pull request Mar 19, 2019

Release 2.4.0
- New Database Wizard [#1952]
- Advanced Search [#1797]
- Automatic update checker [#2648]
- KeeShare database synchronization [#2109, #1992, #2738, #2742, #2746, #2739]
- Improve favicon fetching; transition to Duck-Duck-Go [#2795, #2011, #2439]
- Remove KeePassHttp support [#1752]
- CLI: output info to stderr for easier scripting [#2558]
- CLI: Add --quiet option [#2507]
- CLI: Add create command [#2540]
- CLI: Add recursive listing of entries [#2345]
- CLI: Fix stdin/stdout encoding on Windows [#2425]
- SSH Agent: Support OpenSSH for Windows [#1994]
- macOS: TouchID Quick Unlock [#1851]
- macOS: Multiple improvements; include CLI in DMG [#2165, #2331, #2583]
- Linux: Prevent Klipper from storing secrets in clipboard [#1969]
- Linux: Use polling based file watching for NFS [#2171]
- Linux: Enable use of browser plugin in Snap build [#2802]
- TOTP QR Code Generator [#1167]
- High-DPI Scaling for 4k screens [#2404]
- Make keyboard shortcuts more consistent [#2431]
- Warn user if deleting referenced entries [#1744]
- Allow toolbar to be hidden and repositioned [#1819, #2357]
- Increase max allowed database timeout to 12 hours [#2173]
- Password generator uses existing password length by default [#2318]
- Improve alert message box button labels [#2376]
- Show message when a database merge makes no changes [#2551]
- Browser Integration Enhancements [#1497, #2253, #1904, #2232, #1850, #2218, #2391, #2396, #2542, #2622, #2637, #2790]
- Overall Code Improvements [#2316, #2284, #2351, #2402, #2410, #2419, #2422, #2443, #2491, #2506, #2610, #2667, #2709, #2731]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.