[cfe-dev] [RFC] Improving import failure checking strategy inside ASTImporter

[George Karpenkov via cfe-dev]

> On May 1, 2018, at 3:22 PM, Alexey Sidorin <alexey.v.sidorin at ya.ru> wrote:
> 
> Hi George,
> 
> You have wrote exactly the first idea I have described in the initial proposal:
> > 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.
> 
> 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.
> 
> 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.

Right, I see, sorry for misunderstanding. 
The advantage of using “error” passed by reference is that it could be reused across multiple `.import` calls;
having 50+ nested import calls could be mitigated by wrapping related groups in functions,
that still seems to me a lesser evil than macros.

> 
> 01.05.2018 23:43, George Karpenkov via cfe-dev пишет:
>> Hi Aleksei,
>>>>>> 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.
>> 
>> Macros are indeed ugly, and exceptions are even worse.
>> I think a better solution would be to return an Optional, and make `Importer.Import` accept an out-parameter `Error`, passed by reference.
>> Then you could write:
>> 
>> Optional<Expr*> VisitCXXMemberCallExpr(CXXMemberCallExpr *E) {
>>   Error e;
>>   if (auto T = Importer.import(E->getType(), e))
>>     if (auto ToFn = Importer.import(E->getCallee(), e))
>>       if (auto ToArgs = Importer.import(E->arguments(), e)
>>         return new (Importer.getToContext()) CXXMemberCallExpr(…)
>>   outE = e;
>>   return none;
>> }
>> 
>> That’s even more concise than your example, explicit and does not need macros.
>> 
>> This would be less verbose in C++17 where you would not need nested if’s,
>> but sadly we don’t have that in LLVM yet.	
>> 
>> George
>> 
>> 
>> _______________________________________________
>> cfe-dev mailing list
>> cfe-dev at lists.llvm.org <mailto:cfe-dev at lists.llvm.org>
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev <http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev>
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20180501/c2e22684/attachment.html>