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

[Alexey Sidorin via cfe-dev]

Some import helpers can make our life a bit easier. Some examples below:

     template <typename DeclTy>
     LLVM_NODISCARD bool importInto(DeclTy *&ToD, DeclTy *FromD) {
       if (auto Res = Importer.Import(FromD)) {
         ToD = cast_or_null<DeclTy>(*Res);
         return false;
       }
       return true;
     }

     template <typename DeclTy>
     LLVM_NODISCARD bool importIntoNonNull(DeclTy *&ToD, DeclTy *FromD) {
       if (auto Res = Importer.Import(FromD)) {
         ToD = cast<DeclTy>(*Res);
         return false;
       }
       return true;
     }

     template <typename DeclTy> Optional<DeclTy *> importCasted(DeclTy 
*FromD) {
       if (auto Res = Importer.Import(FromD))
         return cast_or_null<DeclTy>(*Res);
       return None;
     }

     template <typename DeclTy>
     Optional<DeclTy *> importCastedNonNull(DeclTy *FromD) {
       if (auto Res = Importer.Import(FromD))
         return cast<DeclTy>(*Res);
       return None;
     }

They can be used like this:

QualType ASTNodeImporter::VisitTypedefType(const TypedefType *T) {
   if (auto ToDecl = importCastedNonNull(T->getDecl()))
     return Importer.getToContext().getTypeDeclType(*ToDecl);
   return {};
}

...


   if (importInto(ToEPI.ExceptionSpec.SourceDecl,
                  FromEPI.ExceptionSpec.SourceDecl))
     return {};

   if (importInto(ToEPI.ExceptionSpec.SourceTemplate,
                  FromEPI.ExceptionSpec.SourceTemplate))
     return {};

...


QualType ASTNodeImporter::VisitUnresolvedUsingType(
     const UnresolvedUsingType *T) {
   UnresolvedUsingTypenameDecl *ToD, *ToPrevD, *FromD = T->getDecl();
   if (importInto(ToD, FromD) || importInto(ToPrevD, 
FromD->getPreviousDecl()))
     return {};

   return Importer.getToContext().getTypeDeclType(ToD, ToPrevD);
}

Unfortunately, LLVM_NODISCARD is only enabled for C++14 which is not our 
case.



02.05.2018 01:28, George Karpenkov пишет:
>
>
>> On May 1, 2018, at 3:22 PM, Alexey Sidorin <alexey.v.sidorin at ya.ru 
>> <mailto: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
>>> 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/20180518/e95ab6bf/attachment.html>