Add built_value test macro fields. #145
Conversation
davidmorgan
force-pushed
the
built-value-fields
branch
from
594b4fb to
506822e
Compare
davidmorgan
force-pushed
the
built-value-fields
branch
from
506822e to
a005818
Compare
| for (final field in fields) { | ||
| final qualifiedName = field.value.returnType.asNamedTypeDesc.name; | ||
| if (qualifiedName.uri != 'dart:core') { | ||
| fieldTypes.add(qualifiedName.asString); |
There was a problem hiding this comment.
Why are we adding these as strings and then parsing them below?
There was a problem hiding this comment.
Because QualifiedName is an extension type we can't dedupe it with a set, and have to go via String in order to do the query once per type instead of once per field.
There was a problem hiding this comment.
Oh, hmm that is unfortunate. It seems likely this would be a pretty common use case.
Related question... are we in a "Scope.macro" scope here? If so, when you call QualifiedName.parse, is that actually allocating an unused map into the JsonBuffer?
There was a problem hiding this comment.
Yes ... it's similarly annoying in the _resolveNames method in macro_implementation.dart.
And, yes, it ends up in the JsonBuffer. Definitely room for improvement :)
For this specific case though they should just all be fetched by one initial query, i.e. we want a way to say "and metadata of all field types". Or maybe "any metadata called BuiltValue of all field types"...
There was a problem hiding this comment.
Definitely seems like something to be done. Not sure what exactly though, don't have any ideas at the moment. It isn't something to block this PR on though in any case.
There was a problem hiding this comment.
Thanks--I see you opened an issue, we can figure it out there :)
I think this is enough to be interesting for performance questions.
In particular it checks whether field types are
@BuiltValueclasses, which obviously should use a more advanced query to be fast.