<html><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On Aug 5, 2008, at 11:40 AM, Nico Weber wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><span class="Apple-style-span" style="border-collapse: separate; color: rgb(0, 0, 0); font-family: Helvetica; font-size: 13px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-border-horizontal-spacing: 0px; -webkit-border-vertical-spacing: 0px; -webkit-text-decorations-in-effect: none; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0; ">I thought about that, but I don't like bools in constructors. It's ok if they come from a variable, but when I see code like<br><br> TextDiagnosticPrinter printer(true, false);<br><br>then I wonder what those bools do. Most clients will use the default parameters anyways.</span></blockquote><div><br></div><div>I agree there are tradeoffs. The good thing about putting the bools into the ctor is that:</div><div><br></div><div>(a) a constructed TextDiagnosticPrinter object is complete once its constructed. While not so important here, it can be very useful for ensuring correctness in the code as an object is not created in an incomplete state.</div><div><br></div><div>(b) the code is more succinct</div><div><br></div><div>In general, the problem you describe doesn't just apply to bools, but also to places like where you can pass NULL as a parameter value:</div><div><br></div><div>foo(..., NULL, NULL)</div><div><br></div><div>C++ doesn't have labeled function/method calls, so at some point this is just an inherit limitation in the language.</div><div><br></div><div>Another strategy is to use enums instead of bools, e.g:</div><div><br></div><div>TextDiagnosticPrinter* p =</div><div> new TextDiagnosticPrinter( TextDiagnosticPrinter::DisplayCarrots, TextDiagnosticPrinter::DisplayColumns, ...)</div><div><br></div><div>This is more verbose, but harnesses type checking to make sure people pass the right options (in this example DisplayCarrots and DisplayColumns would be enum constants from two separate enum declarations within TextDiagnosticPrinter).</div><div><br></div><blockquote type="cite"><span class="Apple-style-span" style="border-collapse: separate; color: rgb(0, 0, 0); font-family: Helvetica; font-size: 13px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-border-horizontal-spacing: 0px; -webkit-border-vertical-spacing: 0px; -webkit-text-decorations-in-effect: none; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0; ">Furthermore, TDP has an error stream as parameter that defaults to cerr. Should that be first or last? Both uses seem equally likely, so there should probably at least two constructors.</span></blockquote><div><br></div><div>The advantage of putting the options for displaying the columns/carrots first is that people have to thing about the form of the output they want when using a different output steam.</div><br><blockquote type="cite"><span class="Apple-style-span" style="border-collapse: separate; color: rgb(0, 0, 0); font-family: Helvetica; font-size: 13px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-border-horizontal-spacing: 0px; -webkit-border-vertical-spacing: 0px; -webkit-text-decorations-in-effect: none; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0; "><br><br>But if this is the clang way, I won't object. Updated patch (with just this change) is attached.</span></blockquote><br></div><div>Nuno just applied a patch that fixed a bug in TextDiagnosticPrinter:</div><div><br></div><div> <a href="http://llvm.org/viewvc/llvm-project?rev=54365&view=rev">http://llvm.org/viewvc/llvm-project?rev=54365&view=rev</a></div><div><br></div><div>Please include those changes in your patch (if you haven't already) and apply the patch.</div><div><br></div><div>Thanks Nico!</div><div><br></div><div>Ted</div></body></html>