Conversation
rust-cloud-vms
bot
force-pushed
the
mcp-963
branch
from
c8886e5 to
9362166
Compare
reddevilmidzy
changed the title
Tidy: disallow TODO in other in-tree projects Fixes: #152280 MCP: rust-lang/compiler-team#963 TODO * [x] Add ci check to `cg_clif`: rust-lang/rustc_codegen_cranelift#1632 * [x] Add ci check to `cg_gcc`: rust-lang/rustc_codegen_gcc#861 r? lcnr
Tidy: disallow TODO in other in-tree projects Fixes: rust-lang/rust#152280 MCP: rust-lang/compiler-team#963 TODO * [x] Add ci check to `cg_clif`: rust-lang/rustc_codegen_cranelift#1632 * [x] Add ci check to `cg_gcc`: rust-lang/rustc_codegen_gcc#861 r? lcnr
Tidy: disallow TODO in other in-tree projects Fixes: rust-lang/rust#152280 MCP: rust-lang/compiler-team#963 TODO * [x] Add ci check to `cg_clif`: rust-lang/rustc_codegen_cranelift#1632 * [x] Add ci check to `cg_gcc`: rust-lang/rustc_codegen_gcc#861 r? lcnr
rust-cloud-vms
bot
force-pushed
the
mcp-963
branch
from
9362166 to
fe88232
Compare
build_system/todo.rs
Outdated
| if is_editor_temp(&file) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
This is already excluded by list_tracked_files, right?
| let files = list_tracked_files()?; | ||
| let mut errors = Vec::new(); | ||
| // Avoid embedding the task marker in source so greps only find real occurrences. | ||
| let todo_marker = "todo".to_ascii_uppercase(); |
There was a problem hiding this comment.
You are both excluding this file from the todo check and also avoid the literal string TODO. One of the two seems redundant.
There was a problem hiding this comment.
I used lowercase so that grepping for the literal string TODO under the ./compiler paths in the rust-lang/rust repo wouldn’t match this file.
There was a problem hiding this comment.
Ah, SKIP_FILES is not needed anymore, I removed it.
It was left over from using TODO in the previous implementation... haha
build_system/todo.rs
Outdated
| } | ||
|
|
||
| let bytes = | ||
| fs::read(&file).map_err(|e| format!("Failed to read `{}`: {e}", file.display()))?; |
There was a problem hiding this comment.
No need to do error handling. Just unwrap. The build system doesn't have to be robust against hardware/OS issues. It isn't robust in other places either.
build_system/todo.rs
Outdated
| continue; | ||
| } | ||
| let path = std::str::from_utf8(entry) | ||
| .map_err(|_| "Non-utf8 path returned by `git ls-files`".to_string())?; |
build_system/main.rs
Outdated
| if let Err(err) = todo::run() { | ||
| eprintln!("{err}"); | ||
| process::exit(1); | ||
| } | ||
| process::exit(0); |
There was a problem hiding this comment.
Maybe make todo::run() return ! and move the process::exit() into it?
build_system/todo.rs
Outdated
| for (i, line) in contents.split('\n').enumerate() { | ||
| let trimmed = line.trim(); | ||
| if trimmed.contains(&todo_marker) { | ||
| errors.push(format!( |
There was a problem hiding this comment.
This can immediately print the error and increment an error counter.
build_system/todo.rs
Outdated
| fs::read(&file).map_err(|e| format!("Failed to read `{}`: {e}", file.display()))?; | ||
| let Ok(contents) = std::str::from_utf8(&bytes) else { | ||
| continue; | ||
| }; |
There was a problem hiding this comment.
All source files should be valid UTF-8, so unwrap here is fine.
rust-cloud-vms
bot
force-pushed
the
mcp-963
branch
from
fe88232 to
dbbd109
Compare
rust-cloud-vms
bot
force-pushed
the
mcp-963
branch
from
dbbd109 to
7280b4e
Compare
Tidy: disallow TODO in other in-tree projects Fixes: rust-lang/rust#152280 MCP: rust-lang/compiler-team#963 TODO * [x] Add ci check to `cg_clif`: #1632 * [x] Add ci check to `cg_gcc`: rust-lang/rustc_codegen_gcc#861 r? lcnr
2 participants
cc: rust-lang/rust#153166
MCP: rust-lang/compiler-team#963
After the commit rust-lang/rust@69f9c6a is merged here, CI will pass.