-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Rewrite the compiletest directive parser #128070
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,169 @@ | ||
#![allow(unused)] | ||
|
||
mod itemlist; | ||
mod prepare; | ||
|
||
use error::{Error, ErrorExt, Result}; | ||
|
||
#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] | ||
pub struct LineCol { | ||
pub line: usize, | ||
pub col: usize, | ||
} | ||
|
||
impl LineCol { | ||
const fn new(line: usize, col: usize) -> Self { | ||
Self { line, col } | ||
} | ||
} | ||
|
||
/// When we need to e.g. build regexes that include a pattern, we need to know what kind of | ||
/// comments to use. Usually we just build a regex for all expressions, even though we don't | ||
/// use them. | ||
#[derive(Clone, Copy, Debug, PartialEq, Eq)] | ||
enum CommentTy { | ||
Slashes, | ||
Hash, | ||
Semi, | ||
} | ||
|
||
impl CommentTy { | ||
const fn as_str(self) -> &'static str { | ||
match self { | ||
CommentTy::Slashes => "//", | ||
CommentTy::Hash => "#", | ||
CommentTy::Semi => ";", | ||
} | ||
} | ||
|
||
const fn directive(self) -> &'static str { | ||
match self { | ||
CommentTy::Slashes => "//@", | ||
CommentTy::Hash => "#@", | ||
CommentTy::Semi => ";@", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question: I don't think I've ported any There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was thinking LLVM of IR, but duh that's not our job to test 🤦 |
||
} | ||
} | ||
|
||
const fn all() -> &'static [Self] { | ||
&[Self::Slashes, Self::Hash, Self::Semi] | ||
} | ||
} | ||
|
||
/// Errors used within the `load_cfg` module | ||
mod error { | ||
use std::fmt; | ||
use std::path::{Path, PathBuf}; | ||
|
||
use super::LineCol; | ||
|
||
pub type Error = Box<dyn std::error::Error>; | ||
pub type Result<T, E = Error> = std::result::Result<T, E>; | ||
Comment on lines
+59
to
+60
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remark: we might even use some kind of anyhow/thiserror in the future. |
||
|
||
#[derive(Debug)] | ||
struct FullError { | ||
msg: Box<str>, | ||
fname: Option<PathBuf>, | ||
pos: Option<LineCol>, | ||
context: Vec<Box<str>>, | ||
} | ||
Comment on lines
+62
to
+68
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remark: I really like having better context for error messages 👍 |
||
|
||
impl fmt::Display for FullError { | ||
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
write!(f, "error: {}", self.msg)?; | ||
|
||
let path = self.fname.as_ref().map_or(Path::new("unknown").display(), |p| p.display()); | ||
write!(f, "\n parsing '{path}'",)?; | ||
|
||
let pos = self.pos.unwrap_or_default(); | ||
write!(f, " at line {}, column {}", pos.line, pos.col)?; | ||
|
||
if !self.context.is_empty() { | ||
write!(f, "\ncontext: {:#?}", self.context)?; | ||
} | ||
|
||
Ok(()) | ||
} | ||
} | ||
|
||
impl std::error::Error for FullError {} | ||
|
||
/// Give us an easy way to tack context onto an error. | ||
pub trait ErrorExt { | ||
fn pos(self, pos: LineCol) -> Self; | ||
fn line(self, line: usize) -> Self; | ||
fn col(self, col: usize) -> Self; | ||
fn fname(self, fname: impl Into<PathBuf>) -> Self; | ||
fn context(self, ctx: impl Into<Box<str>>) -> Self; | ||
} | ||
|
||
impl ErrorExt for Error { | ||
fn pos(self, pos: LineCol) -> Self { | ||
let mut fe = to_fullerr(self); | ||
fe.pos = Some(pos); | ||
fe | ||
} | ||
|
||
fn line(self, line: usize) -> Self { | ||
let mut fe = to_fullerr(self); | ||
match fe.pos.as_mut() { | ||
Some(v) => v.line = line, | ||
None => fe.pos = Some(LineCol::new(line, 0)), | ||
}; | ||
fe | ||
} | ||
|
||
fn col(self, col: usize) -> Self { | ||
let mut fe = to_fullerr(self); | ||
match fe.pos.as_mut() { | ||
Some(v) => v.col = col, | ||
None => fe.pos = Some(LineCol::new(0, col)), | ||
}; | ||
fe | ||
} | ||
|
||
fn fname(self, fname: impl Into<PathBuf>) -> Self { | ||
let mut fe = to_fullerr(self); | ||
fe.fname = Some(fname.into()); | ||
fe | ||
} | ||
|
||
fn context(self, ctx: impl Into<Box<str>>) -> Self { | ||
let mut fe = to_fullerr(self); | ||
fe.context.push(ctx.into()); | ||
fe | ||
} | ||
} | ||
|
||
impl<T> ErrorExt for Result<T> { | ||
fn pos(self, pos: LineCol) -> Self { | ||
self.map_err(|e| e.pos(pos)) | ||
} | ||
|
||
fn line(self, line: usize) -> Self { | ||
self.map_err(|e| e.line(line)) | ||
} | ||
|
||
fn col(self, col: usize) -> Self { | ||
self.map_err(|e| e.col(col)) | ||
} | ||
|
||
fn fname(self, fname: impl Into<PathBuf>) -> Self { | ||
self.map_err(|e| e.fname(fname)) | ||
} | ||
|
||
fn context(self, ctx: impl Into<Box<str>>) -> Self { | ||
self.map_err(|e| e.context(ctx)) | ||
} | ||
} | ||
|
||
fn to_fullerr(e: Error) -> Box<FullError> { | ||
e.downcast().unwrap_or_else(|e| { | ||
Box::new(FullError { | ||
msg: e.to_string().into(), | ||
fname: None, | ||
pos: None, | ||
context: Vec::new(), | ||
}) | ||
}) | ||
} | ||
} |
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.
Remark: unfortunately,
Makefile
s are still using the same directive prefix#
as comments#
, because I didn't try to port them when I ported//
->//@
since we're portingMakefile
s intormake.rs
anyway.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.
That is an interesting twist - I think it would be easy enough to just ignore unknown directives for specific kinds of tests rather than raising errors. Unless #121876 happens to be done sooner than expected.
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.
Yes, that makes sense to me. We don't really need to error for existing Makefile tests, and at least we are guarding the test suite with a tidy check to disallow new Makefiles to make it in.