Skip to content
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

Don't abuse the file name global.rs #1190

Open
wks opened this issue Aug 28, 2024 · 2 comments
Open

Don't abuse the file name global.rs #1190

wks opened this issue Aug 28, 2024 · 2 comments

Comments

@wks
Copy link
Collaborator

wks commented Aug 28, 2024

In Rust, the idiomatic place to put top-level items (functions, types, constants, ...) of a module is mod.rs.

In mmtk-core, many modules contain global.rs. The file name global.rs is intended for plans, policies, allocators and other things that contain global parts (data structures not specific to any mutator), mutator-specific parts, and GC worker-specific parts. For example, the Immix plan has global.rs, mutator.rs and gc_work.rs. That basically follows the pattern in JikesRVM MMTk, for example, Immix, ImmixMutator, ImmixCollector and ImmixTraceLocal. (Actually in Rust MMTk, global.rs basically does everything. gc_work.rs simply declares what ProcessEdgesWork to use. mutator.rs simply creates Mutator instances, and everything else is done by the Mutator itself.)

But the filename global.rs does not make sense for other things that don't have such distinction. For example,

  • src/plan/global.rs: It simply defines the Plan trait and related top-level types such as CommonPlan and BasePlan. But it also contains things like fn create_mutator, fn create_gc_work_context and AllocationSemantics which obviously don't belong to the global part of a plan.
  • src/util/metadata/global.rs: This file defines enum MetadataSpec and its methods. It is neutral to whether the metadata is global or local. Instead, object_model.rs defines concrete metadata types, including both global and local metadata.
  • src/util/metadata/global.rs: This is for side metadata, regardless whether it is global or local..

The three places above may be abusing the name global.rs for module-level items, and should be in mod.rs, instead.

@qinsoon
Copy link
Member

qinsoon commented Aug 28, 2024

The plan modules were previously named as plan::plan (src/plan/plan.rs), plan::semispace::semispace (src/plan/semispace/semispace.rs), etc. This mostly follows what Java MMTk names plans. However, clippy does not like this. So the inner modules are renamed to global. It actually makes sense, as what are defined in the plan are the global-level stuff.

The global.rs files in metadata are simply a misuse.

We should definitely fix the latter. We may keep the former, or change it -- I don't mind too much.

@qinsoon
Copy link
Member

qinsoon commented Aug 28, 2024

More places with the filename global.rs:

  • src/policy/marksweep/native_ms/global.rs
  • src/policy/marksweep/malloc_ms/global.rs

These are wrong as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants