<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On Sep 14, 2010, at 9:05 AM, Ted Kremenek wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div><br>On Sep 14, 2010, at 12:07 AM, Marcin Świderski wrote:<br><br><blockquote type="cite">Hi<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">This patch implements named parameter idiom for CFG::buildCFG options.<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">Cheers,<br></blockquote><blockquote type="cite">Marcin<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">PS. I can't see messages I send on cfe-commits. Should I send patches to cfe-dev?<br></blockquote><blockquote type="cite"><cfg-build-options.patch><br></blockquote><br>Looks great. Minor nit:<br><br>+ bool PruneTriviallyFalseEdges:1;<br>+ bool AddEHEdges:1;<br>+ bool AddInitializers:1;<br>+ bool AddImplicitDtors:1;<br><br>These probably need to be 'unsigned' instead of 'bool'. I vaguely recall bool bitfields to be a problem for Visual C++ (treats them as signed?). The Clang codebase seems inconsistent, so I'm going to ask a question on cfe-dev.</div></blockquote></div><br><div>On second thought, I think this is fine, since the issue was with enum bitfields, not bool.</div><div><br></div><div>One other comment:</div><div><br></div><div><blockquote type="cite"><div><font class="Apple-style-span" color="#000000">+ class BuildOptions {</font></div><div><font class="Apple-style-span" color="#000000">+ public:</font></div><div><font class="Apple-style-span" color="#000000">+ bool PruneTriviallyFalseEdges:1;</font></div><div><font class="Apple-style-span" color="#000000">+ bool AddEHEdges:1;</font></div><div><font class="Apple-style-span" color="#000000">+ bool AddInitializers:1;</font></div><div><font class="Apple-style-span" color="#000000">+ bool AddImplicitDtors:1;</font></div><div><font class="Apple-style-span" color="#000000">+</font></div><div><font class="Apple-style-span" color="#000000">+ BuildOptions()</font></div><div><font class="Apple-style-span" color="#000000">+ : PruneTriviallyFalseEdges(true)</font></div><div><font class="Apple-style-span" color="#000000">+ , AddEHEdges(false)</font></div><div><font class="Apple-style-span" color="#000000">+ , AddInitializers(false)</font></div><div><font class="Apple-style-span" color="#000000">+ , AddImplicitDtors(false) {}</font></div><div><font class="Apple-style-span" color="#000000">+ BuildOptions setPruneTriviallyFalseEdges(bool v) {</font></div><div><font class="Apple-style-span" color="#000000">+ PruneTriviallyFalseEdges = v;</font></div><div><font class="Apple-style-span" color="#000000">+ return *this;</font></div><div><font class="Apple-style-span" color="#000000">+ }</font></div><div><font class="Apple-style-span" color="#000000">+ BuildOptions setAddEHEdges(bool v) {</font></div><div><font class="Apple-style-span" color="#000000">+ AddEHEdges = v;</font></div><div><font class="Apple-style-span" color="#000000">+ return *this;</font></div><div><font class="Apple-style-span" color="#000000">+ }</font></div><div><font class="Apple-style-span" color="#000000">+ BuildOptions setAddInitializers(bool v) {</font></div><div><font class="Apple-style-span" color="#000000">+ AddInitializers = v;</font></div><div><font class="Apple-style-span" color="#000000">+ return *this;</font></div><div><font class="Apple-style-span" color="#000000">+ }</font></div><div><font class="Apple-style-span" color="#000000">+ BuildOptions setAddImplicitDtors(bool v) {</font></div><div><font class="Apple-style-span" color="#000000">+ AddImplicitDtors = v;</font></div><div><font class="Apple-style-span" color="#000000">+ return *this;</font></div><div><font class="Apple-style-span" color="#000000">+ }</font></div><div><font class="Apple-style-span" color="#000000">+ };</font></div></blockquote><br></div><div>If the bool fields are public, why have the setters? While the following is elegant:</div><div><br></div><div><div>+ CFG::BuildOptions().setPruneTriviallyFalseEdges(false)</div><div>+ .setAddEHEdges(AddEHEdges));</div></div><div><br></div><div>It's probably clearer just to write this as:</div><div><br></div><div>CFG::BuildOptions B;</div><div>B.PruneTriviallyFalseEdges = false;</div><div>B.AddEHEdges = AddEHEdges;</div></body></html>