<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">18.05.2018 03:02, Alexey Sidorin via
cfe-dev пишет:<br>
</div>
<blockquote type="cite"
cite="mid:6996e1f5-7b08-2370-8662-bf25763a909e@ya.ru">
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<div class="moz-cite-prefix">Some import helpers can make our life
a bit easier. Some examples below:<br>
<br>
template <typename DeclTy><br>
LLVM_NODISCARD bool importInto(DeclTy *&ToD, DeclTy
*FromD) {<br>
if (auto Res = Importer.Import(FromD)) {<br>
ToD = cast_or_null<DeclTy>(*Res);<br>
return false;<br>
}<br>
return true;<br>
}<br>
<br>
template <typename DeclTy><br>
LLVM_NODISCARD bool importIntoNonNull(DeclTy *&ToD,
DeclTy *FromD) {<br>
if (auto Res = Importer.Import(FromD)) {<br>
ToD = cast<DeclTy>(*Res);<br>
return false;<br>
}<br>
return true;<br>
}<br>
<br>
template <typename DeclTy> Optional<DeclTy *>
importCasted(DeclTy *FromD) {<br>
if (auto Res = Importer.Import(FromD))<br>
return cast_or_null<DeclTy>(*Res);<br>
return None;<br>
}<br>
<br>
template <typename DeclTy><br>
Optional<DeclTy *> importCastedNonNull(DeclTy *FromD)
{<br>
if (auto Res = Importer.Import(FromD))<br>
return cast<DeclTy>(*Res);<br>
return None;<br>
}<br>
<br>
They can be used like this:<br>
<br>
QualType ASTNodeImporter::VisitTypedefType(const TypedefType *T)
{<br>
if (auto ToDecl = importCastedNonNull(T->getDecl()))<br>
return Importer.getToContext().getTypeDeclType(*ToDecl);<br>
return {};<br>
}<br>
<br>
...<br>
<br>
<br>
if (importInto(ToEPI.ExceptionSpec.SourceDecl,<br>
FromEPI.ExceptionSpec.SourceDecl))<br>
return {};<br>
<br>
if (importInto(ToEPI.ExceptionSpec.SourceTemplate,<br>
FromEPI.ExceptionSpec.SourceTemplate))<br>
return {};<br>
<br>
...<br>
<br>
<br>
QualType ASTNodeImporter::VisitUnresolvedUsingType(<br>
const UnresolvedUsingType *T) {<br>
UnresolvedUsingTypenameDecl *ToD, *ToPrevD, *FromD =
T->getDecl();<br>
if (importInto(ToD, FromD) || importInto(ToPrevD,
FromD->getPreviousDecl()))<br>
return {};<br>
<br>
return Importer.getToContext().getTypeDeclType(ToD, ToPrevD);<br>
}<br>
<br>
Unfortunately, LLVM_NODISCARD is only enabled for C++14 which is
not our case.<br>
</div>
</blockquote>
Sorry, LLVM_DISCARD is fine, I was just mislead.<br>
<br>
<blockquote type="cite"
cite="mid:6996e1f5-7b08-2370-8662-bf25763a909e@ya.ru">
<div class="moz-cite-prefix"> <br>
<br>
<br>
02.05.2018 01:28, George Karpenkov пишет:<br>
</div>
<blockquote type="cite"
cite="mid:EEA62F4F-E159-48DA-BC63-86F587D2AAB5@apple.com">
<meta http-equiv="Content-Type" content="text/html;
charset=utf-8">
<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=""
moz-do-not-send="true">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 class="" wrap="">_______________________________________________
cfe-dev mailing list
<a class="moz-txt-link-abbreviated" href="mailto:cfe-dev@lists.llvm.org" moz-do-not-send="true">cfe-dev@lists.llvm.org</a>
<a class="moz-txt-link-freetext" href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev" moz-do-not-send="true">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev</a>
</pre>
</blockquote>
<p class=""><br class="">
</p>
</div>
</div>
</blockquote>
</div>
<br class="">
</blockquote>
<p><br>
</p>
<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>