-
Notifications
You must be signed in to change notification settings - Fork 562
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Moved defs
plugins files to be based on external files.
#6225
Conversation
13d6d58
to
405e9dc
Compare
2c81f5e
to
0d5e4f3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 8 of 12 files at r1, all commit messages.
Reviewable status: 8 of 12 files reviewed, 1 unresolved discussion (waiting on @Arcticae, @Draggu, @gilbens-starkware, @orizi, and @piotmag769)
crates/cairo-lang-filesystem/src/db.rs
line 115 at r1 (raw file):
/// Returns the virtual file matching the external id. fn ext_as_virtual(&self, _external_id: salsa::InternId) -> VirtualFile { panic!("Should not be called, unless specifically implemented!");
Suggestion:
unreachable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 8 of 12 files reviewed, 1 unresolved discussion (waiting on @Arcticae, @Draggu, @gilbens-starkware, @mkaput, and @piotmag769)
crates/cairo-lang-filesystem/src/db.rs
line 115 at r1 (raw file):
/// Returns the virtual file matching the external id. fn ext_as_virtual(&self, _external_id: salsa::InternId) -> VirtualFile { panic!("Should not be called, unless specifically implemented!");
it is reachable with bad config - definitely should be panic and not unreachable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 4 of 12 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Arcticae, @Draggu, @gilbens-starkware, @orizi, and @piotmag769)
crates/cairo-lang-defs/src/ids.rs
line 308 at r1 (raw file):
/// An id for a file defined out of the filesystem crate. #[derive(Clone, Debug, Hash, PartialEq, Eq)] pub struct ExtFileLongId(pub ModuleId, pub SyntaxStablePtrId, pub SmolStr);
use struct format. from bare declaration I have no idea that that SmolStr
stores
Suggestion:
pub struct ExtFileLongId {
pub module: ModuleId,
pub whose_stable_ptr?: SyntaxStablePtrId,
pub what_is_this?: SmolStr
}
crates/cairo-lang-defs/src/ids.rs
line 309 at r1 (raw file):
#[derive(Clone, Debug, Hash, PartialEq, Eq)] pub struct ExtFileLongId(pub ModuleId, pub SyntaxStablePtrId, pub SmolStr); define_short_id!(ExtFileId, ExtFileLongId, DefsGroup, lookup_intern_ext_file, intern_ext_file);
TBH the choice of term external
is pretty confusing to my eyes. I doubt this can ever be used for other purposed than storing plugin-generated content, so perhaps should all these types names reflect this?
Suggestion:
#[derive(Clone, Debug, Hash, PartialEq, Eq)]
pub struct PluginGeneratedFileLongId(pub ModuleId, pub SyntaxStablePtrId, pub SmolStr);
define_short_id!(PluginGeneratedFileId, PluginGeneratedFileLongId, DefsGroup, lookup_intern_plugin_generated_file, intern_plugin_generated_file);
405e9dc
to
09ddce6
Compare
0d5e4f3
to
50801f7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 10 of 12 files reviewed, 2 unresolved discussions (waiting on @Arcticae, @Draggu, @gilbens-starkware, @mkaput, and @piotmag769)
crates/cairo-lang-defs/src/ids.rs
line 308 at r1 (raw file):
Previously, mkaput (Marek Kaput) wrote…
use struct format. from bare declaration I have no idea that that
SmolStr
stores
Done.
crates/cairo-lang-defs/src/ids.rs
line 309 at r1 (raw file):
Previously, mkaput (Marek Kaput) wrote…
TBH the choice of term
external
is pretty confusing to my eyes. I doubt this can ever be used for other purposed than storing plugin-generated content, so perhaps should all these types names reflect this?
possibly wrong - it very well may be generated for other sort of generated file - but for the time being this would work out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 10 of 12 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Arcticae, @Draggu, @mkaput, @orizi, and @piotmag769)
crates/cairo-lang-defs/src/db.rs
line 621 at r2 (raw file):
}; let mut files = OrderedHashMap::<_, _>::default();
Suggestion:
let mut files = OrderedHashMap:::default();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Arcticae, @Draggu, @gilbens-starkware, @mkaput, and @piotmag769)
crates/cairo-lang-defs/src/db.rs
line 621 at r2 (raw file):
}; let mut files = OrderedHashMap::<_, _>::default();
this would not compile.
09ddce6
to
cb2a0ff
Compare
80ee82b
to
c7728cf
Compare
cb2a0ff
to
f334f07
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Arcticae, @Draggu, @gilbens-starkware, and @piotmag769)
crates/cairo-lang-defs/src/ids.rs
line 309 at r1 (raw file):
Previously, orizi wrote…
possibly wrong - it very well may be generated for other sort of generated file - but for the time being this would work out.
a) it could, but we can think about this when this happens
b) it took me a time to understand what actually external
means => this was a confusing term :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Arcticae, @Draggu, and @piotmag769)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Arcticae, @Draggu, and @piotmag769)
c7728cf
to
5765685
Compare
commit-id:9e8383ea
f334f07
to
915cb03
Compare
Stack:
defs
plugins files to be based on external files. #6225 ⬅This change is