<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Nov 4, 2014 at 4:11 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class="">On Tue, Nov 4, 2014 at 4:07 PM, Alexander Kornienko <span dir="ltr"><<a href="mailto:alexfh@google.com" target="_blank">alexfh@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">The intent of this code is to allow handling of three distinct cases:<br> 1. a valid ClangTidyOptions instance<div> 2. an error with a message</div><div> 3. no error and no ClangTidyOptions, so the code just goes on looking for a configuration file without displaying an error</div><div><br></div><div>Case 3 could be handled by introducing a separate return-by-reference boolean flag or something like that, but here I preferred to use a valid state of the ErrorOr class: HasError + std::error_code containing 0 (success). </div></div></blockquote></span><div><br>Yeah, that seems kind of... not good.<br><br>I'd model this as ErrorOr<Nullable<ClangTidyOptions>> if ClangTidyOptions doesn't have a valid null state that you could use for (3).<br></div></div></div></div></blockquote><div><br></div><div><span style="font-family:arial,sans-serif;font-size:13px">ClangTidyOptions consists of Optionals and one map, so when default initialized, it's actually empty. However, checking for this state would require either an equality operator or a dedicated empty() method. Not a big deal, but the ErrorOr solution seems more straightforward. If we consider this state of ErrorOr illegal, we should change the ErrorOr class to avoid it.</span></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> </div><div><div class="h5"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div>If this looks confusing, I can add a comment describing this case.<div><div><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Nov 4, 2014 at 12:24 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>On Tue, Nov 4, 2014 at 12:23 PM, Justin Bogner <span dir="ltr"><<a href="mailto:mail@justinbogner.com" target="_blank">mail@justinbogner.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span>David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> writes:<br>
> This is totally wrong. ErrorOr's implicit bool conversion is true iff<br>
> there is an error,<br>
><br>
> Doesn't look like it:<br>
><br>
> ErrorOr.h:<br>
> /// \brief Return false if there is an error.<br>
> LLVM_EXPLICIT operator bool() const {<br>
> return !HasError;<br>
> }<br>
<br>
</span>Oops, I misinterpreted "An implicit conversion to bool provides a way to<br>
check if there was an error." - I'll probably clarify that RSN. In any<br>
case, isn't the added check here redundant then?<br></blockquote><div><br></div></span><div>Yep, looks redundant to me.</div><span><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
Maybe it would avoid confusion to use this (fairly common) pattern:<br>
<span><br>
llvm::ErrorOr<ClangTidyOptions> ParsedOptions =<br>
ConfigHandler.second((*Text)->getBuffer());<br>
</span> if (std::error_code EC = ParsedOptions.getError()) {<br></blockquote></span><div><br>Agreed, this seems fairly canonical.<br> </div><span><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
// ...<br>
<div><div> }<br>
<br>
><br>
> so !ParsedOptions implies !ParsedOptions.getError().<br>
><br>
> I think you want:<br>
><br>
> llvm::ErrorOr<ClangTidyOptions> ParsedOptions =<br>
> ConfigHandler.second((*Text)->getBuffer());<br>
> if (ParsedOptions) {<br>
> llvm::errs() << "Error parsing " << ConfigFile << ": "<br>
> << ParsedOptions.getError().message() << "\n";<br>
> ...<br>
><br>
> This obviously changes the behaviour, but the current behaviour doesn't<br>
> make sense.<br>
><br>
> > continue;<br>
> > }<br>
> ><br>
> ><br>
> ><br>
> > _______________________________________________<br>
> > cfe-commits mailing list<br>
> > <a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
> > <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
> _______________________________________________<br>
> cfe-commits mailing list<br>
> <a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</div></div></blockquote></span></div></div></div></blockquote></div></div></div></div></div></div></blockquote></div></div></div></div></div></blockquote></div><br>
</div></div>