Spud

Merge lp:~cwilson/spud/pointers_to_incomplete_type into lp:spud

  1. pointers_to_incomplete_type
  2. Merge into trunk
Proposed by Cian Wilson
Status: Merged
Merged at revision: 537
Proposed branch: lp:~cwilson/spud/pointers_to_incomplete_type
Merge into: lp:spud
Diff against target: 335 lines (+73/-68)
2 files modified
include/spud (+7/-7)
src/spud.cpp (+66/-61)
To merge this branch: bzr merge lp:~cwilson/spud/pointers_to_incomplete_type
Reviewer Review Type Date Requested Status
Stephan Kramer Approve
Review via email: mp+267956@code.launchpad.net

Description of the change

I've been trying to compile spud using clang on mac OSX 10.10 (Yosemite) and ran into a few problems. This branch fixes them...

1. The Option class depends on storing a std::deque of std::pairs of std::strings and Options. This involves instantiating a deque using an incomplete type (as you're passing an Option type to the pair and deque from within the definition of the Option type). The behaviour of this is apparently undefined - the stdlib implementation we've been using on ubuntu has been able to deal with it but the llvm/clang/mac implementation complains:
"error: field has incomplete type 'Spud::OptionManager::Option'"
To deal with this I've changed the deque to store pairs of strings and pointers to Options. I've therefore also changed the destructors to delete these pointers.

2. spud.cpp was segfaulting at line 1456 (in the new branch). This was because child had been left pointing to children.end(), a new child had then been pushed back onto children and it was assumed that child (pointing to children.end()) would now point at the new entry in children, which appears not to be true with this stdlib implementation. A small fix just updates child to point at the new final entry in the children deque.

I've manually merged these changes into fluidity:
https://github.com/FluidityProject/fluidity/tree/cianwilson/spudchanges
(note that I've only merged these changes on top of what appears to be an out of date spud in fluidity). This passes make test locally and is being run through the buildbot here:
http://buildbot.ese.ic.ac.uk:8080/waterfall?show=cianwilson/spudchanges

To post a comment you must log in.
Revision history for this message
Cian Wilson (cwilson) wrote :

Green (fluidity) buildbot.

Revision history for this message
Stephan Kramer (s-kramer) wrote :

I've read it through (learning some c++ on the way) and it looks like a sensible solution to the described problem. Tested as well as is feasible. So I'd say: good to go!

review: Approve
Revision history for this message
Cian Wilson (cwilson) wrote :

Thanks Stephan!

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'include/spud'
2--- include/spud 2011-07-28 13:17:39 +0000
3+++ include/spud 2015-08-13 14:29:36 +0000
4@@ -192,15 +192,15 @@
5
6 /** Finds first element with this key.
7 */
8- std::deque< std::pair<std::string, Option> >::iterator find(const std::string& key);
9- std::deque< std::pair<std::string, Option> >::const_iterator find(const std::string& key) const;
10+ std::deque< std::pair<std::string, Option*> >::iterator find(const std::string& key);
11+ std::deque< std::pair<std::string, Option*> >::const_iterator find(const std::string& key) const;
12
13 /** Finds next element with this key after current iterator.
14 */
15- std::deque< std::pair<std::string, Option> >::iterator
16- find_next(std::deque< std::pair<std::string, Option> >::iterator current, const std::string& key);
17- std::deque< std::pair<std::string, Option> >::const_iterator
18- find_next(std::deque< std::pair<std::string, Option> >::const_iterator current, const std::string& key) const;
19+ std::deque< std::pair<std::string, Option*> >::iterator
20+ find_next(std::deque< std::pair<std::string, Option*> >::iterator current, const std::string& key);
21+ std::deque< std::pair<std::string, Option*> >::const_iterator
22+ find_next(std::deque< std::pair<std::string, Option*> >::const_iterator current, const std::string& key) const;
23
24 /**
25 * Get the number of elements at the supplied key. Searches all
26@@ -393,7 +393,7 @@
27 std::string data_as_string() const;
28
29 std::string node_name;
30- std::deque< std::pair<std::string, Option> > children;
31+ std::deque< std::pair<std::string, Option*> > children;
32
33 int rank, shape[2];
34 std::vector<double> data_double;
35
36=== modified file 'src/spud.cpp'
37--- src/spud.cpp 2011-07-28 14:48:25 +0000
38+++ src/spud.cpp 2015-08-13 14:29:36 +0000
39@@ -673,6 +673,10 @@
40 }
41
42 OptionManager::Option::~Option(){
43+ for(deque< pair<string, Option*> >::iterator it=children.begin();it!=children.end();++it){
44+ delete it->second;
45+ }
46+
47 return;
48 }
49
50@@ -779,7 +783,7 @@
51
52 const Option* descendant = get_child(name);
53 if(descendant != NULL){
54- for(deque< pair<string, Option> >::const_iterator it = descendant->children.begin();it != descendant->children.end();it++){
55+ for(deque< pair<string, Option*> >::const_iterator it = descendant->children.begin();it != descendant->children.end();it++){
56 kids.push_back(it->first);
57 }
58 }
59@@ -789,31 +793,31 @@
60
61 size_t OptionManager::Option::count(const string& key) const{
62 size_t cnt=0;
63- for(deque< pair<string, Option> >::const_iterator it=children.begin();it!=children.end();++it){
64+ for(deque< pair<string, Option*> >::const_iterator it=children.begin();it!=children.end();++it){
65 if(it->first == key)
66 cnt++;
67 }
68 return cnt;
69 }
70
71- deque< pair<string, OptionManager::Option> >::iterator OptionManager::Option::find(const string& key){
72- for(deque< pair<string, Option> >::iterator it=children.begin();it!=children.end();++it){
73- if(it->first == key)
74- return it;
75- }
76- return children.end();
77- }
78-
79- deque< pair<string, OptionManager::Option> >::const_iterator OptionManager::Option::find(const string& key) const{
80- for(deque< pair<string, Option> >::const_iterator it=children.begin();it!=children.end();++it){
81- if(it->first == key)
82- return it;
83- }
84- return children.end();
85- }
86-
87- deque< pair<string, OptionManager::Option> >::iterator OptionManager::Option::find_next(deque< pair<string, OptionManager::Option> >::iterator current, const string& key){
88- deque< pair<string, OptionManager::Option> >::iterator it = current;
89+ deque< pair<string, OptionManager::Option*> >::iterator OptionManager::Option::find(const string& key){
90+ for(deque< pair<string, Option*> >::iterator it=children.begin();it!=children.end();++it){
91+ if(it->first == key)
92+ return it;
93+ }
94+ return children.end();
95+ }
96+
97+ deque< pair<string, OptionManager::Option*> >::const_iterator OptionManager::Option::find(const string& key) const{
98+ for(deque< pair<string, Option*> >::const_iterator it=children.begin();it!=children.end();++it){
99+ if(it->first == key)
100+ return it;
101+ }
102+ return children.end();
103+ }
104+
105+ deque< pair<string, OptionManager::Option*> >::iterator OptionManager::Option::find_next(deque< pair<string, OptionManager::Option*> >::iterator current, const string& key){
106+ deque< pair<string, OptionManager::Option*> >::iterator it = current;
107 it++;
108 for(;it!=children.end();++it){
109 if(it->first == key)
110@@ -822,8 +826,8 @@
111 return children.end();
112 }
113
114- deque< pair<string, OptionManager::Option> >::const_iterator OptionManager::Option::find_next(deque< pair<string, OptionManager::Option> >::const_iterator current, const string& key) const{
115- deque< pair<string, Option> >::const_iterator it = current;
116+ deque< pair<string, OptionManager::Option*> >::const_iterator OptionManager::Option::find_next(deque< pair<string, OptionManager::Option*> >::const_iterator current, const string& key) const{
117+ deque< pair<string, Option*> >::const_iterator it = current;
118 it++;
119 for(;it!=children.end();++it){
120 if(it->first == key)
121@@ -850,7 +854,7 @@
122 return NULL;
123 }
124
125- deque< pair<string, Option> >::const_iterator it;
126+ deque< pair<string, Option*> >::const_iterator it;
127 if(count(name) == 0){
128 name += "::";
129 int i = 0;
130@@ -880,9 +884,9 @@
131 if(it == children.end()){
132 return NULL;
133 }else if(branch.empty()){
134- return &(it->second);
135+ return it->second;
136 }else{
137- return it->second.get_child(branch);
138+ return it->second->get_child(branch);
139 }
140 }
141
142@@ -920,20 +924,20 @@
143 }
144
145 int count = 0, i = 0;
146- for(deque< pair<string, Option> >::const_iterator it = children.begin();it != children.end();it++){
147+ for(deque< pair<string, Option*> >::const_iterator it = children.begin();it != children.end();it++){
148 if(it->first.compare(0, name.size(), name) == 0 and (not match_whole or it->first.size() == name.size())){
149 if(index < 0){
150 if(branch.empty()){
151 count++;
152 }else{
153- count += it->second.option_count(branch);
154+ count += it->second->option_count(branch);
155 }
156 }else{
157 if(i == index){
158 if(branch.empty()){
159 count++;
160 }else{
161- count += it->second.option_count(branch);
162+ count += it->second->option_count(branch);
163 }
164 break;
165 }
166@@ -961,7 +965,7 @@
167 cout << "OptionType OptionManager::Option::get_option_type(void) const\n";
168
169 if(have_option("__value")){
170- return find("__value")->second.get_option_type();
171+ return find("__value")->second->get_option_type();
172 }
173
174 if(!data_double.empty()){
175@@ -980,7 +984,7 @@
176 cout << "size_t OptionManager::Option::get_option_rank(void) const\n";
177
178 if(have_option("__value")){
179- return find("__value")->second.get_option_rank();
180+ return find("__value")->second->get_option_rank();
181 }else{
182 return rank;
183 }
184@@ -991,7 +995,7 @@
185 cout << "vector<int> OptionManager::Option::get_option_shape(void) const\n";
186
187 if(have_option("__value")){
188- return find("__value")->second.get_option_shape();
189+ return find("__value")->second->get_option_shape();
190 }else{
191 vector<int> shape(2);
192 shape[0] = this->shape[0];
193@@ -1234,15 +1238,15 @@
194 return SPUD_KEY_ERROR;
195 }
196
197- Option new_option1 = Option(*option1);
198- new_option1.node_name = key2_name;
199+ Option* new_option1 = new Option(*option1);
200+ new_option1->node_name = key2_name;
201 string new_node_name, name_attr;
202- new_option1.split_node_name(new_node_name, name_attr);
203+ new_option1->split_node_name(new_node_name, name_attr);
204 if(name_attr.size() == 0){
205- option2_parent->children.push_back(pair<string, Option>(new_node_name, new_option1));
206+ option2_parent->children.push_back(pair<string, Option*>(new_node_name, new_option1));
207 }else{
208- new_option1.set_attribute("name", name_attr);
209- option2_parent->children.push_back(pair<string, Option>(new_node_name + "::" + name_attr, new_option1));
210+ new_option1->set_attribute("name", name_attr);
211+ option2_parent->children.push_back(pair<string, Option*>(new_node_name + "::" + name_attr, new_option1));
212 }
213
214 return SPUD_NO_ERROR;
215@@ -1272,15 +1276,15 @@
216 return SPUD_KEY_ERROR;
217 }
218
219- Option new_option1 = Option(*option1);
220- new_option1.node_name = key2_name;
221+ Option* new_option1 = new Option(*option1);
222+ new_option1->node_name = key2_name;
223 string new_node_name, name_attr;
224- new_option1.split_node_name(new_node_name, name_attr);
225+ new_option1->split_node_name(new_node_name, name_attr);
226 if(name_attr.size() == 0){
227- option2_parent->children.push_back(pair<string, Option>(new_node_name, new_option1));
228+ option2_parent->children.push_back(pair<string, Option*>(new_node_name, new_option1));
229 }else{
230- new_option1.set_attribute("name", name_attr);
231- option2_parent->children.push_back(pair<string, Option>(new_node_name + "::" + name_attr, new_option1));
232+ new_option1->set_attribute("name", name_attr);
233+ option2_parent->children.push_back(pair<string, Option*>(new_node_name + "::" + name_attr, new_option1));
234 }
235
236 delete_option(key1);
237@@ -1302,16 +1306,16 @@
238 if(opt == NULL){
239 return SPUD_KEY_ERROR;
240 }else if(branch.empty()){
241- deque< pair<string, Option> >::iterator iter = find(name);
242+ deque< pair<string, Option*> >::iterator iter = find(name);
243 while(iter!=children.end()){
244- if(&iter->second == opt){
245+ if(iter->second == opt){
246 children.erase(iter);
247 return SPUD_NO_ERROR;
248 }
249 iter = find_next(iter, name);
250 }
251 for(iter = children.begin();iter != children.end();iter++){
252- if(&iter->second == opt){
253+ if(iter->second == opt){
254 children.erase(iter);
255 return SPUD_NO_ERROR;
256 }
257@@ -1361,8 +1365,8 @@
258 cout << lprefix << "<value>: " << data_string;
259 cout << endl;
260 }
261- for(deque< pair<string, Option> >::const_iterator i = children.begin();i!=children.end();++i){
262- i->second.print(lprefix + " ");
263+ for(deque< pair<string, Option*> >::const_iterator i = children.begin();i!=children.end();++i){
264+ i->second->print(lprefix + " ");
265 }
266 }
267
268@@ -1399,7 +1403,7 @@
269 return NULL;
270 }
271
272- deque< pair<string, Option> >::iterator child;
273+ deque< pair<string, Option*> >::iterator child;
274 if(count(name) == 0){
275 string name2 = name + "::";
276 int i = 0;
277@@ -1417,11 +1421,12 @@
278 cerr << "SPUD WARNING: Creating __value child for non null element - deleting parent data" << endl;
279 set_option_type(SPUD_NONE);
280 }
281- children.push_back(pair<string, Option>(name, Option(name)));
282+ children.push_back(pair<string, Option*>(name, new Option(name)));
283 string new_node_name, name_attr;
284- children.rbegin()->second.split_node_name(new_node_name, name_attr);
285+ child = children.end(); child--;
286+ child->second->split_node_name(new_node_name, name_attr);
287 if(name_attr.size() > 0){
288- children.rbegin()->second.set_attribute("name", name_attr);
289+ child->second->set_attribute("name", name_attr);
290 }
291 is_attribute = false;
292 }
293@@ -1438,7 +1443,7 @@
294 child = find_next(child, name);
295 }
296 if(child == children.end() and index == (int)i){
297- children.push_back(pair<string, Option>(name, Option(name)));
298+ children.push_back(pair<string, Option*>(name, new Option(name)));
299 child = children.end(); child--;
300 is_attribute = false;
301 }
302@@ -1448,9 +1453,9 @@
303 if(child == children.end()){
304 return NULL;
305 }else if(branch.empty()){
306- return &child->second;
307+ return child->second;
308 }else{
309- return child->second.create_child(branch);
310+ return child->second->create_child(branch);
311 }
312 }
313
314@@ -1700,15 +1705,15 @@
315 data_ele->SetValue(data_as_string());
316 ele->LinkEndChild(data_ele);
317
318- for(deque< pair<string, Option> >::const_iterator iter = children.begin();iter != children.end();iter++){
319- if(iter->second.is_attribute){
320+ for(deque< pair<string, Option*> >::const_iterator iter = children.begin();iter != children.end();iter++){
321+ if(iter->second->is_attribute){
322 // Add attribute
323- ele->SetAttribute(iter->second.node_name, iter->second.data_as_string());
324+ ele->SetAttribute(iter->second->node_name, iter->second->data_as_string());
325 }else{
326- TiXmlElement* child_ele = iter->second.to_element();
327- if(iter->second.node_name == "__value"){
328+ TiXmlElement* child_ele = iter->second->to_element();
329+ if(iter->second->node_name == "__value"){
330 // Detect data sub-element
331- switch(iter->second.get_option_type()){
332+ switch(iter->second->get_option_type()){
333 case(SPUD_DOUBLE):
334 child_ele->SetValue("real_value");
335 break;

Subscribers

People subscribed via source and target branches