This is an archive of the discontinued LLVM Phabricator instance.

[ASTImporter] Friend class decl should not be visible in its context
ClosedPublic

Authored by martong on Dec 4 2019, 8:17 AM.

Details

Summary

In the past we had to use DeclContext::makeDeclVisibleInContext to make
friend declarations available for subsequent lookup calls and this way
we could chain (redecl) the structurally equivalent decls.
By doing this we created an AST that improperly made declarations
visible in some contexts, so the AST was malformed.
Since we use the importer specific lookup this is no longer necessary,
because with that we can find every previous nodes.

Event Timeline

martong created this revision.Dec 4 2019, 8:17 AM
Herald added a reviewer: shafik. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript

Hi Gabor,
This patch looks mostly good to me. Thanks!

clang/lib/AST/ASTImporter.cpp
334

llvm::is_contained?

clang/unittests/AST/ASTImporterTest.cpp
258

Why don't we use getCanonicalType() anymore?

martong updated this revision to Diff 233675.Dec 12 2019, 1:25 PM
martong marked 4 inline comments as done.

Addressing Alexeis comments.

clang/lib/AST/ASTImporter.cpp
334

Thanks! Good catch.

clang/unittests/AST/ASTImporterTest.cpp
258

Thanks, that is a very good catch.

Hi Gabor,
I have an inline comment for the patch. Otherwise, looks good.

clang/unittests/AST/ASTImporterTest.cpp
250

I wonder if ElaboratedType can be a canonical type because it is only a sugar type itself. Do we need to handle this case?

martong updated this revision to Diff 234062.Dec 16 2019, 7:43 AM
martong marked an inline comment as done.

Simplify getRecordDeclOfFriend() further.

martong marked an inline comment as done.Dec 16 2019, 7:43 AM
martong added inline comments.
clang/unittests/AST/ASTImporterTest.cpp
250

You are right, I removed this case. Thanks!

a_sidorin accepted this revision.Dec 16 2019, 1:17 PM

LGTM, thanks!

This revision is now accepted and ready to land.Dec 16 2019, 1:17 PM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Dec 17 2019, 10:41 AM

Looks like this breaks building on Windows in general: http://45.33.8.238/win/4247/step_4.txt

Recommited in bc5b7e21e32 . I changed llvm:is_contained to a simple for loop over the lookup result. This way the copy assignment of the iterator is avoided even if windows STL is used.