<div class="gmail_quote">W dniu 14 września 2010 18:14 użytkownik Ted Kremenek <span dir="ltr"><<a href="mailto:kremenek@cs.stanford.edu">kremenek@cs.stanford.edu</a>></span> napisał:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div style="word-wrap:break-word"><div><div></div><div class="h5"><br><div><div>On Sep 14, 2010, at 9:05 AM, Ted Kremenek wrote:</div><br><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></div><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 color="#000000">+  class BuildOptions {</font></div><div><font color="#000000">+  public:</font></div><div class="im"><div><font color="#000000">+    bool PruneTriviallyFalseEdges:1;</font></div><div><font color="#000000">+    bool AddEHEdges:1;</font></div>
<div><font color="#000000">+    bool AddInitializers:1;</font></div><div><font color="#000000">+    bool AddImplicitDtors:1;</font></div></div><div><font color="#000000">+</font></div><div><font color="#000000">+    BuildOptions()</font></div>
<div><font color="#000000">+        : PruneTriviallyFalseEdges(true)</font></div><div><font color="#000000">+        , AddEHEdges(false)</font></div><div><font color="#000000">+        , AddInitializers(false)</font></div>
<div><font color="#000000">+        , AddImplicitDtors(false) {}</font></div><div><font color="#000000">+    BuildOptions setPruneTriviallyFalseEdges(bool v) {</font></div><div><font color="#000000">+      PruneTriviallyFalseEdges = v;</font></div>
<div><font color="#000000">+      return *this;</font></div><div><font color="#000000">+    }</font></div><div><font color="#000000">+    BuildOptions setAddEHEdges(bool v) {</font></div><div><font color="#000000">+      AddEHEdges = v;</font></div>
<div><font color="#000000">+      return *this;</font></div><div><font color="#000000">+    }</font></div><div><font color="#000000">+    BuildOptions setAddInitializers(bool v) {</font></div><div><font color="#000000">+      AddInitializers = v;</font></div>
<div><font color="#000000">+      return *this;</font></div><div><font color="#000000">+    }</font></div><div><font color="#000000">+    BuildOptions setAddImplicitDtors(bool v) {</font></div><div><font color="#000000">+      AddImplicitDtors = v;</font></div>
<div><font color="#000000">+      return *this;</font></div><div><font color="#000000">+    }</font></div><div><font 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></div></blockquote><div> </div></div>I'm used to using named parameters object as temporary created in function's parameters list, just like I did in AnalysisContext.cpp . Such approach is clearer IMO, because it shows exactly what I want to achieve: pass parameters to function, and not create an object named B and pass it to function.<div>
<br></div><div>But that's just my opinion and I can remove those setters if it will better fit the whole project.</div>