This is an archive of the discontinued LLVM Phabricator instance.

[ASTImporter] Support import of CXXDeductionGuideDecl
ClosedPublic

Authored by martong on Nov 25 2020, 9:33 AM.

Details

Summary

CXXDeductionGuideDecl is a FunctionDecl, but its constructor should be called
appropriately, at least to set the kind variable properly.

Diff Detail

Event Timeline

martong created this revision.Nov 25 2020, 9:33 AM
martong requested review of this revision.Nov 25 2020, 9:33 AM
teemperor requested changes to this revision.Nov 25 2020, 10:55 AM

You think it's worth it to have a test for a user-specified deduction guide? I think this is missing an 'explicit' deduction guide test, so maybe we could test both things in a single additional test case.

Also, IIUC we ignore the IsCopyDeductionCandidate bit when importing and that seems like a potential bug to me (the value is apparently used for some overload resolution logic)?

Beside those two things this looks good to me.

clang/lib/AST/ASTImporter.cpp
3332

typo "funcitons"

This revision now requires changes to proceed.Nov 25 2020, 10:55 AM
martong updated this revision to Diff 307848.Nov 26 2020, 5:44 AM
  • Add test for user-defined ctad
  • Handle IsCopyDeductionCandidate
  • Fix typo

Thanks for the review! Your comment made me think about whether we handle the deduced template class properly (getDeducedTemplate). Then I realized we do import that when we import the DeclarationName. Anyway I added a check for that in the test.

teemperor accepted this revision.Nov 26 2020, 6:59 AM

I think the test could be made a bit stricter (see inline comment), but otherwise this LGTM. Thanks!

clang/unittests/AST/ASTImporterTest.cpp
5973

Not sure if that's actually testing what it should. FromD is not the copy deduction candidate, so this is just comparing that both Decls have the default value false?

If we pick the copy deduction candidate in this test then we could test that we actually copy the value over and not just have the default 'false' flag for isCopyDeductionCandidate (something like cxxDeductionGuideDecl(hasParameter(0, hasType(asString("A<T>")))) should find the copy deduction candidate).

Then we could also simplify this to EXPECT_TRUE(ToD->isCopyDeductionCandidate());.

This revision is now accepted and ready to land.Nov 26 2020, 6:59 AM
martong updated this revision to Diff 307988.Nov 27 2020, 1:37 AM
martong marked 2 inline comments as done.
  • Update test to match the copy deduction guide
clang/unittests/AST/ASTImporterTest.cpp
5973

Yeah, good point, updated so.

teemperor accepted this revision.Nov 27 2020, 2:35 AM
This revision was landed with ongoing or failed builds.Nov 30 2020, 8:56 AM
This revision was automatically updated to reflect the committed changes.
rnk added a subscriber: rnk.Nov 30 2020, 4:36 PM
rnk added inline comments.
clang/lib/AST/ASTImporter.cpp
3336

GCC 5.3 complained about this, so I rewrote it a different way without generic lambdas in rG43b5b485a203f190ee4d5d3cab19c44ca865d316.

Error spew:

FAILED: tools/clang/lib/AST/CMakeFiles/obj.clangAST.dir/ASTImporter.cpp.o 
/b/s/w/ir/cache/builder/src/third_party/llvm-build-tools/gcc530trusty/bin/g++  -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Itools/clang/lib/AST -I/b/s/w/ir/cache/builder/src/third_party/llvm/clang/lib/AST -I/b/s/w/ir/cache/builder/src/third_party/llvm/clang/include -Itools/clang/include -Iinclude -I/b/s/w/ir/cache/builder/src/third_party/llvm/llvm/include -DLLVM_FORCE_HEAD_REVISION -fvisibility-inlines-hidden -Werror=date-time -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wno-maybe-uninitialized -Wdelete-non-virtual-dtor -Wno-comment -fdiagnostics-color -ffunction-sections -fdata-sections -fno-common -Woverloaded-virtual -fno-strict-aliasing -O3     -fno-exceptions -fno-unwind-tables -fno-asynchronous-unwind-tables -fno-rtti -UNDEBUG -std=c++14 -MD -MT tools/clang/lib/AST/CMakeFiles/obj.clangAST.dir/ASTImporter.cpp.o -MF tools/clang/lib/AST/CMakeFiles/obj.clangAST.dir/ASTImporter.cpp.o.d -o tools/clang/lib/AST/CMakeFiles/obj.clangAST.dir/ASTImporter.cpp.o -c /b/s/w/ir/cache/builder/src/third_party/llvm/clang/lib/AST/ASTImporter.cpp
/b/s/w/ir/cache/builder/src/third_party/llvm/clang/lib/AST/ASTImporter.cpp: In instantiation of ‘clang::ASTNodeImporter::VisitFunctionDecl(clang::FunctionDecl*)::<lambda(auto:1*)> [with auto:1 = clang::CXXConversionDecl; clang::ExpectedExpr = llvm::Expected<clang::Expr*>]’:
/b/s/w/ir/cache/builder/src/third_party/llvm/clang/lib/AST/ASTImporter.cpp:3384:66:   required from here
/b/s/w/ir/cache/builder/src/third_party/llvm/clang/lib/AST/ASTImporter.cpp:3336:20: error: cannot call member function ‘T clang::ASTNodeImporter::importChecked(llvm::Error&, const T&) [with T = clang::Expr*]’ without object
       ExplicitExpr = importChecked(Err, Fun->getExplicitSpecifier().getExpr());
                    ^
/b/s/w/ir/cache/builder/src/third_party/llvm/clang/lib/AST/ASTImporter.cpp: In instantiation of ‘clang::ASTNodeImporter::VisitFunctionDecl(clang::FunctionDecl*)::<lambda(auto:1*)> [with auto:1 = clang::CXXDeductionGuideDecl; clang::ExpectedExpr = llvm::Expected<clang::Expr*>]’:
/b/s/w/ir/cache/builder/src/third_party/llvm/clang/lib/AST/ASTImporter.cpp:3402:57:   required from here
/b/s/w/ir/cache/builder/src/third_party/llvm/clang/lib/AST/ASTImporter.cpp:3336:20: error: cannot call member function ‘T clang::ASTNodeImporter::importChecked(llvm::Error&, const T&) [with T = clang::Expr*]’ without object
[3456/5141] Building CXX object tools/clang/lib/AST/CMakeFiles/obj.clangAST.dir/InheritViz.cpp.o
martong added inline comments.Dec 1 2020, 2:12 AM
clang/lib/AST/ASTImporter.cpp
3336

Thanks, post-commit looks good!