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

opt: dictionarygroup refactor in articleview #1625

Merged
merged 10 commits into from
Jul 3, 2024
Merged

Conversation

xiaoyifang
Copy link
Owner

No description provided.

@@ -35,8 +36,7 @@ class ArticleView: public QWidget

ArticleNetworkAccessManager & articleNetMgr;
AudioPlayerPtr const & audioPlayer;
std::vector< sptr< Dictionary::Class > > const & allDictionaries;
Instances::Groups const & groups;
DictionaryGroup dictionaryGroup;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

std::unique_ptr / QScopedPointer seems better than "QObject with this as parent" in here. They manage the lifetime equally, but the former don't have bloats.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

two different ways to the same goal.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For objects with no complex deleter, the cost of unique_ptr is zero.

@xiaoyifang xiaoyifang force-pushed the opt/dictionarygroup branch from ddc5ebb to 96bd752 Compare July 2, 2024 14:26
@xiaoyifang xiaoyifang force-pushed the opt/dictionarygroup branch from a4234f4 to a3dd7ca Compare July 3, 2024 07:16
@xiaoyifang xiaoyifang merged commit 932b88f into staged Jul 3, 2024
8 of 9 checks passed
@xiaoyifang xiaoyifang deleted the opt/dictionarygroup branch July 3, 2024 07:16
Copy link

sonarqubecloud bot commented Jul 3, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants