ModuleSet: store a set rather than a map
#292
Conversation
9999years
force-pushed
the
rebeccat/refactor-module-set
branch
3 times, most recently
from
dc0a15c to
a97ecd3
Compare
9999years
force-pushed
the
rebeccat/refactor-module-set
branch
from
a97ecd3 to
a93cc3f
Compare
Previously, a `ModuleSet` wrapped a `HashMap<NormalPath, TargetKind>`. This had a number of undesirable consequences: * The data about a module's name (as its path) and how it was loaded (as its `TargetKind`) were split from each other and difficult to reference. * The module's import name wasn't explicitly stored anywhere, so we needed to convert between paths and dotted names when those were needed, which required hitting the disk. * There wasn't a type for the module's import name, so when we (e.g.) `:unadd`ed modules we needed to format them as strings. Now, a `ModuleSet` wraps a `HashSet<LoadedModule>`. * A `LoadedModule` wraps a path but optionally contains the module's dotted name, if the module is loaded by name (and needs to be referred to by name to avoid the "module defined in multiple files" error). * The `LoadedModule` `Display` instance formats the module's import name correctly (with a dotted name if needed) and avoids hitting the disk or any string processing.
9999years
force-pushed
the
rebeccat/refactor-module-set
branch
from
a93cc3f to
ffde017
Compare
| /// Create a new module, loaded by path. | ||
| pub fn new(path: NormalPath) -> Self { | ||
| Self { path, name: None } | ||
| } | ||
|
|
||
| /// Create a new module, loaded by name. | ||
| pub fn with_name(path: NormalPath, name: String) -> Self { | ||
| Self { | ||
| path, | ||
| name: Some(name), | ||
| } | ||
| } |
There was a problem hiding this comment.
I think I might have asked this before, but what's the motivation for using these methods instead of using the LoadedModule constructor directly?
There was a problem hiding this comment.
It's the usual encapsulation reason. If you want to be able to write LoadedModule { path, name: None } then all of the struct's fields have to be public (and theoretically callers could use this to break invariants).
In reality, LoadedModule isn't exported publicly by the library and it would be basically fine to have the fields be public, but this is considered "good practice" so I lean towards it.
(Also constructor names can be a little more informative than setting struct fields directly.)
| pub fn path(&self) -> &NormalPath { | ||
| &self.path |
There was a problem hiding this comment.
Similar question here: what's the advantage of doing loadedModule.path() instead of loadedModule.path?
| pub struct ModuleSet { | ||
| modules: HashSet<LoadedModule>, | ||
| } |
There was a problem hiding this comment.
Non-blocking, but I feel like this struct wrapper around the HashSet doesn't buy you much because most of the methods on ModuleSet are trivially wrapping methods on the underlying HashSet. You could probably pass around the HashSet directly instead of defining a separate ModuleSet struct.
There was a problem hiding this comment.
Yeah, that's true. I like the type safety this gives, though, and I like being able to give semantically descriptive names to the methods. I think I'll keep this for now.
|
I tried making the |
Previously, a
ModuleSetwrapped aHashMap<NormalPath, TargetKind>. This had a number of undesirable consequences:TargetKind) were split from each other and difficult to reference.:unadded modules we needed to format them as strings.Now, a
ModuleSetwraps aHashSet<LoadedModule>.LoadedModulewraps a path but optionally contains the module's dotted name, if the module is loaded by name (and needs to be referred to by name to avoid the "module defined in multiple files" error).LoadedModuleDisplayinstance formats the module's import name correctly (with a dotted name if needed) and avoids hitting the disk or any string processing.patch,minor, ormajorto request a version bump when it's merged.docs/.tests/.