Skip to content

Conversation

@adrian-prantl
Copy link

which indirectly caused several ModuleSP shared pointers to live longer than two tests expected them to.

The problem is that swift::performImportResolution() now apparently also reports syntax errors in function bodies, so I had to add a (terrible) heuristic to filter out actual module import errors.

rdar://135575668

which indirectly caused several ModuleSP shared pointers to live
longer than two tests expected them to.

The problem is that swift::performImportResolution() now apparently
also reports syntax errors in function bodies, so I had to add a
(terrible) heuristic to filter out actual module import errors.

rdar://135575668
@adrian-prantl
Copy link
Author

@swift-ci test

std::string msg = llvm::toString(expr_diagnostics.GetAllErrors());
if (StringRef(msg).contains(": could not build module "))
return make_error<ModuleImportError>(msg);
return llvm::createStringError(msg);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: let's std::move(msg) here and above, since those are likely big strings.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this not an optimization that Clang can do on its own?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, not in the middle of an expression. Clang will move when you have a naked return. E.g. imagine you had an Error class (which cannot be copied) instead of a string, this compiles:

return error;

but this doesn't compile:

return foo(error);

Copy link

@felipepiovezan felipepiovezan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems a bit scary that there is a connection between the problem and this fix, but I don't have enough knowledge about these to propose a long term solution.

@felipepiovezan
Copy link

Thank you and Jim for investigating this!

@adrian-prantl
Copy link
Author

It seems a bit scary that there is a connection between the problem and this fix, but I don't have enough knowledge about these to propose a long term solution.

Good question! The fallback mechanism that caused this is a bit of an anachronism in the days of precise compiler invocations, and I'm thinking hard about removing it completely to simplify the code.

@adrian-prantl adrian-prantl merged commit 97e95e0 into swiftlang:stable/20240723 Sep 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants