widelands

Merge lp:~flegu/widelands/r8481-renderedtext-memory-leaks into lp:widelands

  1. r8481-renderedtext-memory-leaks
  2. Merge into trunk
Proposed by Jukka Pakarinen
Status: Merged
Merged at revision: 8485
Proposed branch: lp:~flegu/widelands/r8481-renderedtext-memory-leaks
Merge into: lp:widelands
Diff against target: 131 lines (+18/-18)
2 files modified
src/graphic/text/rendered_text.h (+1/-1)
src/graphic/text/rt_render.cc (+17/-17)
To merge this branch: bzr merge lp:~flegu/widelands/r8481-renderedtext-memory-leaks
Reviewer Review Type Date Requested Status
GunChleoc Approve
Review via email: mp+333578@code.launchpad.net

Description of the change

Valgrind analysis reveals memory leaks from RenderedText objects during startup of the game. The leaks are already reported in bug 1668200 #4 #6.

The leaks are detected in Debug and Release builds on Debian 9.1. Valgrind does not see the leaks if a build is done with the changes in the branch.

To post a comment you must log in.
Revision history for this message
GunChleoc (gunchleoc) wrote :

Thanks for the fix!

Looks good to me - can you please replace UI::RenderedText::Shared with std::shared_ptr<UI::RenderedText> and get rid of the new using statement? I think that would make the code easier to read.

Revision history for this message
Jukka Pakarinen (flegu) wrote :

> Thanks for the fix!
>
> Looks good to me - can you please replace UI::RenderedText::Shared with
> std::shared_ptr<UI::RenderedText> and get rid of the new using statement? I
> think that would make the code easier to read.

Sure, I can do that.

Revision history for this message
GunChleoc (gunchleoc) wrote :

Great, thanks!

How do you want to be credited on the developers list? Real name, nickname or both?

@bunnybot merge

review: Approve
Revision history for this message
Jukka Pakarinen (flegu) wrote :

> How do you want to be credited on the developers list? Real name, nickname or
> both?

I think it is better to leave out the nickname because it isn't the same as my nickname in the list of Finnish translators. Thanks!

Revision history for this message
bunnybot (widelandsofficial) wrote :

Continuous integration builds have changed state:

Travis build 2793. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/300650021.
Appveyor build 2604. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_flegu_widelands_r8481_renderedtext_memory_leaks-2604.

Revision history for this message
bunnybot (widelandsofficial) wrote :

Refusing to merge, since Travis is not green. Use @bunnybot merge force for merging anyways.

Travis build 2793. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/300650021.

Revision history for this message
GunChleoc (gunchleoc) wrote :

@bunnybot merge force

Revision history for this message
GunChleoc (gunchleoc) wrote :

Name added, you should appear on the homepage too the next time it regenerates.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/graphic/text/rendered_text.h'
2--- src/graphic/text/rendered_text.h 2017-06-24 08:47:46 +0000
3+++ src/graphic/text/rendered_text.h 2017-11-11 16:40:48 +0000
4@@ -115,7 +115,7 @@
5 };
6
7 struct RenderedText {
8- /// RenderedRects that can be drawn on screen
9+ /// RenderedRects that can be drawn on screen
10 std::vector<std::unique_ptr<RenderedRect>> rects;
11
12 /// The width occupied by all rects in pixels.
13
14=== modified file 'src/graphic/text/rt_render.cc'
15--- src/graphic/text/rt_render.cc 2017-08-18 14:27:26 +0000
16+++ src/graphic/text/rt_render.cc 2017-11-11 16:40:48 +0000
17@@ -230,7 +230,7 @@
18 virtual uint16_t width() const = 0;
19 virtual uint16_t height() const = 0;
20 virtual uint16_t hotspot_y() const = 0;
21- virtual UI::RenderedText* render(TextureCache* texture_cache) = 0;
22+ virtual std::shared_ptr<UI::RenderedText> render(TextureCache* texture_cache) = 0;
23
24 // TODO(GunChleoc): Remove this function once conversion is finished and well tested.
25 virtual std::string debug_info() const = 0;
26@@ -527,7 +527,7 @@
27 return rv;
28 }
29
30- UI::RenderedText* render(TextureCache* texture_cache) override;
31+ std::shared_ptr<UI::RenderedText> render(TextureCache* texture_cache) override;
32
33 protected:
34 uint16_t w_, h_;
35@@ -550,11 +550,11 @@
36 return font_.ascent(nodestyle_.font_style);
37 }
38
39-UI::RenderedText* TextNode::render(TextureCache* texture_cache) {
40+std::shared_ptr<UI::RenderedText> TextNode::render(TextureCache* texture_cache) {
41 auto rendered_image =
42 font_.render(txt_, nodestyle_.font_color, nodestyle_.font_style, texture_cache);
43 assert(rendered_image.get() != nullptr);
44- UI::RenderedText* rendered_text = new UI::RenderedText();
45+ std::shared_ptr<UI::RenderedText> rendered_text(new UI::RenderedText());
46 rendered_text->rects.push_back(
47 std::unique_ptr<UI::RenderedRect>(new UI::RenderedRect(rendered_image)));
48 return rendered_text;
49@@ -579,7 +579,7 @@
50 return "ft";
51 }
52
53- UI::RenderedText* render(TextureCache*) override;
54+ std::shared_ptr<UI::RenderedText> render(TextureCache*) override;
55
56 bool is_expanding() const override {
57 return is_expanding_;
58@@ -591,8 +591,8 @@
59 private:
60 bool is_expanding_;
61 };
62-UI::RenderedText* FillingTextNode::render(TextureCache* texture_cache) {
63- UI::RenderedText* rendered_text = new UI::RenderedText();
64+std::shared_ptr<UI::RenderedText> FillingTextNode::render(TextureCache* texture_cache) {
65+ std::shared_ptr<UI::RenderedText> rendered_text(new UI::RenderedText());
66 const std::string hash =
67 (boost::format("rt:fill:%s:%s:%i:%i:%i:%s") % txt_ % nodestyle_.font_color.hex_value() %
68 nodestyle_.font_style % width() % height() % (is_expanding_ ? "e" : "f"))
69@@ -633,9 +633,9 @@
70 return "wsp";
71 }
72
73- UI::RenderedText* render(TextureCache* texture_cache) override {
74+ std::shared_ptr<UI::RenderedText> render(TextureCache* texture_cache) override {
75 if (show_spaces_) {
76- UI::RenderedText* rendered_text = new UI::RenderedText();
77+ std::shared_ptr<UI::RenderedText> rendered_text(new UI::RenderedText());
78 const std::string hash = (boost::format("rt:wsp:%i:%i") % width() % height()).str();
79 std::shared_ptr<const Image> rendered_image = texture_cache->get(hash);
80 if (rendered_image.get() == nullptr) {
81@@ -681,7 +681,7 @@
82 uint16_t hotspot_y() const override {
83 return 0;
84 }
85- UI::RenderedText* render(TextureCache* /* texture_cache */) override {
86+ std::shared_ptr<UI::RenderedText> render(TextureCache* /* texture_cache */) override {
87 NEVER_HERE();
88 }
89 bool is_non_mandatory_space() const override {
90@@ -712,8 +712,8 @@
91 uint16_t hotspot_y() const override {
92 return h_;
93 }
94- UI::RenderedText* render(TextureCache* texture_cache) override {
95- UI::RenderedText* rendered_text = new UI::RenderedText();
96+ std::shared_ptr<UI::RenderedText> render(TextureCache* texture_cache) override {
97+ std::shared_ptr<UI::RenderedText> rendered_text(new UI::RenderedText());
98 const std::string hash = (boost::format("rt:sp:%s:%i:%i:%s") % filename_ % width() %
99 height() % (is_expanding_ ? "e" : "f"))
100 .str();
101@@ -802,8 +802,8 @@
102 return desired_width_;
103 }
104
105- UI::RenderedText* render(TextureCache* texture_cache) override {
106- UI::RenderedText* rendered_text = new UI::RenderedText();
107+ std::shared_ptr<UI::RenderedText> render(TextureCache* texture_cache) override {
108+ std::shared_ptr<UI::RenderedText> rendered_text(new UI::RenderedText());
109 // Preserve padding
110 rendered_text->rects.push_back(std::unique_ptr<UI::RenderedRect>(
111 new UI::RenderedRect(Recti(0, 0, width(), height()), nullptr)));
112@@ -912,7 +912,7 @@
113 uint16_t hotspot_y() const override {
114 return scale_ * image_->height();
115 }
116- UI::RenderedText* render(TextureCache* texture_cache) override;
117+ std::shared_ptr<UI::RenderedText> render(TextureCache* texture_cache) override;
118
119 private:
120 const Image* image_;
121@@ -922,8 +922,8 @@
122 bool use_playercolor_;
123 };
124
125-UI::RenderedText* ImgRenderNode::render(TextureCache* texture_cache) {
126- UI::RenderedText* rendered_text = new UI::RenderedText();
127+std::shared_ptr<UI::RenderedText> ImgRenderNode::render(TextureCache* texture_cache) {
128+ std::shared_ptr<UI::RenderedText> rendered_text(new UI::RenderedText());
129
130 if (scale_ == 1.0) {
131 // Image can be used as is, and has already been cached in g_gr->images()

Subscribers

People subscribed via source and target branches

to status/vote changes: