[cfe-dev] [RFC] Improving import failure checking strategy inside ASTImporter
Alexey Sidorin via cfe-dev
cfe-dev at lists.llvm.org
Thu May 17 17:32:50 PDT 2018
18.05.2018 03:02, 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.
Sorry, LLVM_DISCARD is fine, I was just mislead.
>
>
>
> 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
>>>
>>>
>>
>
>
>
> _______________________________________________
> 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/1211ed94/attachment.html>
More information about the cfe-dev
mailing list