<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">Hi George,<br>
<br>
You have wrote exactly the first idea I have described in the
initial proposal:<br>
> 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>
<br>
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>
<br>
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>
<br>
01.05.2018 23:43, George Karpenkov via cfe-dev пишет:<br>
</div>
<blockquote type="cite"
cite="mid:D671D24A-9A21-4B74-8601-3B7A8A55FE11@apple.com">
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<div>
<div>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>
<div><br class="">
</div>
<div>Macros are indeed ugly, and exceptions are even worse.</div>
<div>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>Then you could write:</div>
<div><br class="">
</div>
<div>Optional<Expr*>
VisitCXXMemberCallExpr(CXXMemberCallExpr *E) {</div>
<div> Error e;</div>
<div> if (auto T = Importer.import(E->getType(), e))</div>
<div> if (auto ToFn = Importer.import(E->getCallee(),
e))</div>
<div> if (auto ToArgs =
Importer.import(E->arguments(), e)</div>
<div> return new (Importer.getToContext())
CXXMemberCallExpr(…)</div>
<div> outE = e;</div>
<div> return none;</div>
<div>}</div>
<div><br class="">
</div>
<div>That’s even more concise than your example, explicit and
does not need macros.</div>
<div><br class="">
</div>
<div>This would be less verbose in C++17 where you would not
need nested if’s,</div>
<div>but sadly we don’t have that in LLVM yet.<span class="Apple-tab-span" style="white-space:pre"> </span></div>
<div><br class="">
</div>
<div>George</div>
</div>
</div>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<br>
<pre wrap="">_______________________________________________
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><br>
</p>
</body>
</html>