<html><head><meta http-equiv="Content-Type" content="text/html; charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><br class=""><div><br class=""><blockquote type="cite" class=""><div class="">On May 1, 2018, at 3:22 PM, Alexey Sidorin <<a href="mailto:alexey.v.sidorin@ya.ru" class="">alexey.v.sidorin@ya.ru</a>> wrote:</div><br class="Apple-interchange-newline"><div class="">
<meta http-equiv="Content-Type" content="text/html; charset=utf-8" class="">
<div text="#000000" bgcolor="#FFFFFF" class="">
<div class="moz-cite-prefix">Hi George,<br class="">
<br class="">
You have wrote exactly the first idea I have described in the
initial proposal:<br class="">
> The problem with enforcing checks is the most annoying one
but also looks to be the simplest - we can just replace the return
value with Optional or ErrorOr so clients will be enforced to
perform a failure checking.<br class="">
<br class="">
My question was about avoiding related boilerplate with nested
if's (or lots of early returns). Optionals are excellent if one
needs to do in-place checks. But if we have >50 nested Import()
calls (which is a pretty common case for recursive ASTImporter),
and we want to check only the top-level result, their usage
becomes questionable. However, it still looks to be the best
option we have. Adding an argument passed by reference in addition
to Optional doesn't look good to me as well; moreover, we have
Expected/ErrorOr for these usage scenarios.<br class="">
<br class="">
Anyway, maybe even my question itself is not correct. The question
of error handling doesn't exist on its own, it is the result of
ASTImporter design. Maybe a potential solution is to split the
lookup and structural equivalence checking (to a kind of dry run)
from node import: in this case, Import() is called only after all
checks are done so it cannot fail. But this will need ASTImporter
redesign, and it is definitely not a thing I'm going to work on
soon.<br class=""></div></div></div></blockquote><div><br class=""></div>Right, I see, sorry for misunderstanding. </div><div>The advantage of using “error” passed by reference is that it could be reused across multiple `.import` calls;</div><div>having 50+ nested import calls could be mitigated by wrapping related groups in functions,</div><div>that still seems to me a lesser evil than macros.</div><div><br class=""><blockquote type="cite" class=""><div class=""><div text="#000000" bgcolor="#FFFFFF" class=""><div class="moz-cite-prefix">
<br class="">
01.05.2018 23:43, George Karpenkov via cfe-dev пишет:<br class="">
</div>
<blockquote type="cite" cite="mid:D671D24A-9A21-4B74-8601-3B7A8A55FE11@apple.com" class="">
<meta http-equiv="Content-Type" content="text/html; charset=utf-8" class="">
<div class="">
<div class="">Hi Aleksei,</div>
<blockquote type="cite" class="">
<div class="">
<div style="word-wrap: break-word; -webkit-nbsp-mode: space;
line-break: after-white-space;" class="">
<div class="">
<blockquote type="cite" class="">
<div class="">
<blockquote type="cite" cite="mid:CAH6rKyCELnyVYeguSFfi=i_N+UyVMvJYaO=RnaZM2Mr3q_dCDQ@mail.gmail.com" style="font-family: Helvetica; font-size: 12px;
font-style: normal; font-variant-caps: normal;
font-weight: normal; letter-spacing: normal;
orphans: auto; text-align: start; text-indent:
0px; text-transform: none; white-space: normal;
widows: auto; word-spacing: 0px;
-webkit-text-size-adjust: auto;
-webkit-text-stroke-width: 0px; background-color:
rgb(255, 255, 255); text-decoration: none;" class="">
<blockquote type="cite" class="">
<pre class="" wrap="">The problem of improving readability of these checks is a bit harder. The
most straightforward approach is to create a macro like:
#define IMPORT_PTR_INTO_VAR(VarName, Type, From) \
Type *VarName = nullptr; \
{ \
auto MacroImportRes = Importer.Import(From); \
if (!MacroImportRes) \
return MacroImportRes.getError(); \
VarName = *MacroImportRes; \
}
Same for non-pointer type. So, the final code will look like this:
ErrorOr<Expr *> ASTNodeImporter::VisitCXXMemberCallExpr(CXXMemberCallExpr
*E) {
IMPORT_QUAL_TYPE_INTO_VAR(T, E->getType())
IMPORT_PTR_INTO_VAR(ToFn, Expr, E->getCallee())
SmallVector<Expr *, 4> ToArgs(E->getNumArgs());
IMPORT_CONTAINER(E->arguments(), ToArgs);
return new (Importer.getToContext()) CXXMemberCallExpr(
Importer.getToContext(), ToFn, ToArgs, T, E->getValueKind(),
Importer.Import(E->getRParenLoc()));
}
But macros don't look clean. Unfortunately, I still haven't found a better
option so any suggestions are very welcome.
</pre>
</blockquote>
</blockquote>
</div>
</blockquote>
</div>
</div>
</div>
</blockquote>
<div class="">
<div class=""><br class="">
</div>
<div class="">Macros are indeed ugly, and exceptions are even worse.</div>
<div class="">I think a better solution would be to return an Optional,
and make `Importer.Import` accept an out-parameter `Error`,
passed by reference.</div>
<div class="">Then you could write:</div>
<div class=""><br class="">
</div>
<div class="">Optional<Expr*>
VisitCXXMemberCallExpr(CXXMemberCallExpr *E) {</div>
<div class=""> Error e;</div>
<div class=""> if (auto T = Importer.import(E->getType(), e))</div>
<div class=""> if (auto ToFn = Importer.import(E->getCallee(),
e))</div>
<div class=""> if (auto ToArgs =
Importer.import(E->arguments(), e)</div>
<div class=""> return new (Importer.getToContext())
CXXMemberCallExpr(…)</div>
<div class=""> outE = e;</div>
<div class=""> return none;</div>
<div class="">}</div>
<div class=""><br class="">
</div>
<div class="">That’s even more concise than your example, explicit and
does not need macros.</div>
<div class=""><br class="">
</div>
<div class="">This would be less verbose in C++17 where you would not
need nested if’s,</div>
<div class="">but sadly we don’t have that in LLVM yet.<span class="Apple-tab-span" style="white-space:pre"> </span></div>
<div class=""><br class="">
</div>
<div class="">George</div>
</div>
</div>
<br class="">
<fieldset class="mimeAttachmentHeader"></fieldset>
<br class="">
<pre wrap="" class="">_______________________________________________
cfe-dev mailing list
<a class="moz-txt-link-abbreviated" href="mailto:cfe-dev@lists.llvm.org">cfe-dev@lists.llvm.org</a>
<a class="moz-txt-link-freetext" href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev</a>
</pre>
</blockquote><p class=""><br class="">
</p>
</div>
</div></blockquote></div><br class=""></body></html>