Repository navigation
Get overload to consider compiler flags in cache lookup - #6888
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
stuartarchibald
left a comment
stuartarchibald
left a comment
There was a problem hiding this comment.
Thanks for the patch, I think this fixes the issue with ensuring that overloads are selected based on flags.
| if self: | ||
| flags = self.top() | ||
| else: | ||
| # Note: should this be the default flag for the target instead? |
There was a problem hiding this comment.
I think perhaps it should be the default so it is always known precisely which flags are in use for a given function.
There was a problem hiding this comment.
I'm getting into a rabbit hole trying to put a default flag there. Currently, the TargetOptions is not really part of any contexts. It's floating in the TargetDescriptor. I can put the options into the target-context but it doesn't make sense for it to appear in the typing-context. However, I need it during typing. This goes back to the purity problem of needing flags (which I don't think is part of types) during the typing phase.
The only doable hack I can think of is putting the info into the ConfigStack to read the default flag later.
There was a problem hiding this comment.
hm.... the other way is to let flags be part of the typing context.
There was a problem hiding this comment.
I sampled the testsuite and found that this branch is only reached from test that bypass the user-facing compiler API---e.g. tests that invokes part of the compiler to test for specific internal behavior. The failure to use a default flag will only result in redundant compilation. I don't think we should spend too much time on this problem for now.
There was a problem hiding this comment.
From OOB conversation, agree on leaving it for now as this fixes the immediate issue.
stuartarchibald
left a comment
stuartarchibald
left a comment
There was a problem hiding this comment.
Thanks for the patch. There's a typo to resolve, else I think this should be merged as it'll permit multiple lines of work in figuring out what needs fixing else where!
@sklam IIRC you ran the test suite to check this and None for flags in the cache key was quite rare and seemed like a harmless recompile?
stuartarchibald
added
4 - Waiting on author
Co-authored-by: stuartarchibald <stuartarchibald@users.noreply.github.com>
Correct. It only happens from our testsuite using |
sklam
added
4 - Waiting on reviewer
stuartarchibald
left a comment
stuartarchibald
left a comment
There was a problem hiding this comment.
Thanks for the patch and fixes!
stuartarchibald
added
5 - Ready to merge
2 participants
as titled