Skip to content

Comments

no codegen for parts of failed template instantiations#3074

Merged
9rnsr merged 1 commit intodlang:masterfrom
rainers:donot_write_failed_templates
Jan 10, 2014
Merged

no codegen for parts of failed template instantiations#3074
9rnsr merged 1 commit intodlang:masterfrom
rainers:donot_write_failed_templates

Conversation

@rainers
Copy link
Member

@rainers rainers commented Jan 9, 2014

do not write failed template instantiations or parts of these to the object file.

This solves the duplicate COMDAT error in dlang/phobos#1411

src/dsymbol.c Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Usual style in dmd is if (

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed spacing.

@rainers
Copy link
Member Author

rainers commented Jan 9, 2014

It seems b69eb50 fixed the error propagation, so I have reduced it to a simple function in Dsymbol. The function name isInFailedInstantiation now is too specific, but hasErrors sounds too generic. The patch #2480 uses isInErrorTree but it's ugly. Any better suggestion?

@yebblies
Copy link
Contributor

yebblies commented Jan 9, 2014

Just use a loop in TemplateInstance::toObjFile.

@rainers
Copy link
Member Author

rainers commented Jan 9, 2014

Just use a loop in TemplateInstance::toObjFile

To avoid finding a good name? ;-)

The PR #2480 calls it from other places aswell (e.g. when TypeInfo for erronuous types has been generated), so a function would be nice.

@yebblies
Copy link
Contributor

yebblies commented Jan 9, 2014

The problem is that we shouldn't be getting here in the first place - at this point all instances with errors are speculative and shouldn't be outputted anyway. Maybe we should keep a list of all child template instances and recursively mark them as non-speculative?

https://d.puremagic.com/issues/show_bug.cgi?id=11724

@rainers
Copy link
Member Author

rainers commented Jan 10, 2014

Following Kenjis' suggestion, I moved the parent check to isError.

Copy link
Contributor

Choose a reason for hiding this comment

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

s is already != NULL, so s && is redundant.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmmm, I'd rather say the test is missing in the check for s->errors. If we can assume isErrors is always a Dsymbol, we should pass it as that.

Copy link
Member Author

Choose a reason for hiding this comment

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

BTW: is there any reason why code isn't written as "if (Dsymbol* s = isDsymbol(o)) {}"? I'm very much tempted to change it to this style if I stumble over it.

Copy link
Contributor

Choose a reason for hiding this comment

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

o is neither Type, Expression, nor Tuple. so it is always Dsymbol.

Copy link
Contributor

Choose a reason for hiding this comment

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

Additionally, s->errors at line 105 already assumes that s != NULL. Therefore, testing it here looks weird to me.

@9rnsr
Copy link
Contributor

9rnsr commented Jan 10, 2014

Anyway, this change does not have serious problem, and I also confirmed that the it would fix the issue in #1411 in my local.
LGTM.

9rnsr added a commit that referenced this pull request Jan 10, 2014
no codegen for parts of failed template instantiations
@9rnsr 9rnsr merged commit 062d821 into dlang:master Jan 10, 2014
@rainers
Copy link
Member Author

rainers commented Jan 10, 2014

Thanks. The win64 tests now pass.
I've created a small followup pull request #3079 for the suggested cleanup.

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.

4 participants