Assorted bugfixes for new analysis #14658
Conversation
…nction and Itself
…riant rather than on the type variable
| if (sort.classes.size() != 1 || *sort.classes.begin() != m_analysis.typeSystem().primitiveClass(PrimitiveClass::Type)) | ||
| sortString = ":" + TypeSystemHelpers{m_analysis.typeSystem()}.sortToString(m_analysis.typeSystem().env().sort(type)); | ||
| std::optional<Type> const& inferredType = m_analysis.annotation<TypeInference>(_node).type; | ||
| if (inferredType.has_value()) |
There was a problem hiding this comment.
As for random style remarks: I find .has_value() on optionals horrible myself personally :-D... but it doesn't matter at all, ultimately everybody can read both, so I don't bother pushing my own preferred style through.
There was a problem hiding this comment.
For me it's if (inferredType) that's horrible. It does not read like a condition at all. It forces me stop and think in which way this can be interpreted as a boolean, while on seeing inferredType.has_value() I immediately know it's an optional and can move on smoothly. I'm really not sure why people don't find this jarring.
There was a problem hiding this comment.
That's my point, though - it's purely subjective and we spend way too much time and effort in general in flipping back and forth around purely subjective taste that has no actual bearing on overall readability.
There was a problem hiding this comment.
As in: I find this significantly less readable with .has_value() than without - and I don't buy that it'd be worse for you to read it without that than it is for me to read it with that - we should just generally have a bit more flexibility around stuff like this (that would already significantly reduce number of comments on PRs).
Well, I thought you wanted all of the fixups removed from the base branch :) |
Assorted bugfixes for new analysis
Assorted bugfixes for new analysis
Assorted bugfixes for new analysis
Moved here from my fixups into
newAnalysis:Functiontypes being created, instead ofFunctionandItself.resolveRecursive()not resolving type variables.index()being used on theTypevariant rather thanTypeVariableindextoid? The current name seems very prone to this kind of error.DebugWarner.unify()- should have been an assert.uint64_tinstead ofsize_tused for the index inTypeEnvironment::fresh().size_teverywhere else. This is part of compiler output though, so a portable type likeuint64_tis probably a better choice though. Should we switch?