From 5e7a80b2d2462060e81295ea6a044f7012bba8cb Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Mon, 13 May 2024 08:57:42 +1000 Subject: [PATCH 1/2] Make handling of `Comments` more iterator-like. The current way of stepping through each comment in `Comments` is a bit weird. There is a `Vec` and a `current` index, which is fine. The `Comments::next` method clones the current comment but doesn't advance `current`; the advancing instead happens in `print_comment`, which is where each cloned comment is actually finally used (or not, in some cases, if the comment fails to satisfy a predicate). This commit makes things more iterator-like: - `Comments::next` now advances `current` instead of `print_comment`. - `Comments::peek` is added so you can inspect a comment and check a predicate without consuming it. - This requires splitting `PrintState::comments` into immutable and mutable versions. The commit also moves the ref inside the `Option` of the return type, to save callers from having to use `as_ref`/`as_mut`. - It also requires adding `PrintState::peek_comment` alongside the existing `PrintState::next_comment`. (The lifetimes in the signature of `peek_comment` ended up more complex than I expected.) We now have a neat separation between consuming (`next`) and non-consuming (`peek`) uses of each comment. As well as being clearer, this will facilitate the next commit that avoids unnecessary cloning. --- compiler/rustc_ast_pretty/src/pprust/state.rs | 56 +++++++++++-------- compiler/rustc_hir_pretty/src/lib.rs | 8 ++- 2 files changed, 40 insertions(+), 24 deletions(-) diff --git a/compiler/rustc_ast_pretty/src/pprust/state.rs b/compiler/rustc_ast_pretty/src/pprust/state.rs index 2c176828c84..9eb552cdcdf 100644 --- a/compiler/rustc_ast_pretty/src/pprust/state.rs +++ b/compiler/rustc_ast_pretty/src/pprust/state.rs @@ -186,17 +186,23 @@ pub fn new(sm: &'a SourceMap, filename: FileName, input: String) -> Comments<'a> Comments { sm, comments, current: 0 } } + fn peek(&self) -> Option<&Comment> { + self.comments.get(self.current) + } + // FIXME: This shouldn't probably clone lmao - fn next(&self) -> Option { - self.comments.get(self.current).cloned() + fn next(&mut self) -> Option { + let cmnt = self.comments.get(self.current).cloned(); + self.current += 1; + cmnt } fn trailing_comment( - &self, + &mut self, span: rustc_span::Span, next_pos: Option, ) -> Option { - if let Some(cmnt) = self.next() { + if let Some(cmnt) = self.peek() { if cmnt.style != CommentStyle::Trailing { return None; } @@ -204,7 +210,7 @@ fn trailing_comment( let comment_line = self.sm.lookup_char_pos(cmnt.pos); let next = next_pos.unwrap_or_else(|| cmnt.pos + BytePos(1)); if span.hi() < cmnt.pos && cmnt.pos < next && span_line.line == comment_line.line { - return Some(cmnt); + return Some(self.next().unwrap()); } } @@ -400,7 +406,8 @@ fn deref_mut(&mut self) -> &mut Self::Target { /// This trait is used for both AST and HIR pretty-printing. pub trait PrintState<'a>: std::ops::Deref + std::ops::DerefMut { - fn comments(&mut self) -> &mut Option>; + fn comments(&self) -> Option<&Comments<'a>>; + fn comments_mut(&mut self) -> Option<&mut Comments<'a>>; fn ann_post(&mut self, ident: Ident); fn print_generic_args(&mut self, args: &ast::GenericArgs, colons_before_params: bool); @@ -442,18 +449,18 @@ fn commasep(&mut self, b: Breaks, elts: &[T], op: F) fn maybe_print_comment(&mut self, pos: BytePos) -> bool { let mut has_comment = false; - while let Some(cmnt) = self.next_comment() { - if cmnt.pos < pos { - has_comment = true; - self.print_comment(&cmnt); - } else { + while let Some(cmnt) = self.peek_comment() { + if cmnt.pos >= pos { break; } + has_comment = true; + let cmnt = self.next_comment().unwrap(); + self.print_comment(cmnt); } has_comment } - fn print_comment(&mut self, cmnt: &Comment) { + fn print_comment(&mut self, cmnt: Comment) { match cmnt.style { CommentStyle::Mixed => { if !self.is_beginning_of_line() { @@ -517,19 +524,20 @@ fn print_comment(&mut self, cmnt: &Comment) { self.hardbreak(); } } - if let Some(cmnts) = self.comments() { - cmnts.current += 1; - } + } + + fn peek_comment<'b>(&'b self) -> Option<&'b Comment> where 'a: 'b { + self.comments().and_then(|c| c.peek()) } fn next_comment(&mut self) -> Option { - self.comments().as_mut().and_then(|c| c.next()) + self.comments_mut().and_then(|c| c.next()) } fn maybe_print_trailing_comment(&mut self, span: rustc_span::Span, next_pos: Option) { - if let Some(cmnts) = self.comments() { + if let Some(cmnts) = self.comments_mut() { if let Some(cmnt) = cmnts.trailing_comment(span, next_pos) { - self.print_comment(&cmnt); + self.print_comment(cmnt); } } } @@ -537,11 +545,11 @@ fn maybe_print_trailing_comment(&mut self, span: rustc_span::Span, next_pos: Opt fn print_remaining_comments(&mut self) { // If there aren't any remaining comments, then we need to manually // make sure there is a line break at the end. - if self.next_comment().is_none() { + if self.peek_comment().is_none() { self.hardbreak(); } while let Some(cmnt) = self.next_comment() { - self.print_comment(&cmnt) + self.print_comment(cmnt) } } @@ -994,8 +1002,12 @@ fn to_string(f: impl FnOnce(&mut State<'_>)) -> String { } impl<'a> PrintState<'a> for State<'a> { - fn comments(&mut self) -> &mut Option> { - &mut self.comments + fn comments(&self) -> Option<&Comments<'a>> { + self.comments.as_ref() + } + + fn comments_mut(&mut self) -> Option<&mut Comments<'a>> { + self.comments.as_mut() } fn ann_post(&mut self, ident: Ident) { diff --git a/compiler/rustc_hir_pretty/src/lib.rs b/compiler/rustc_hir_pretty/src/lib.rs index 4f5fbd024a9..a47e6af0bf2 100644 --- a/compiler/rustc_hir_pretty/src/lib.rs +++ b/compiler/rustc_hir_pretty/src/lib.rs @@ -139,8 +139,12 @@ fn deref_mut(&mut self) -> &mut Self::Target { } impl<'a> PrintState<'a> for State<'a> { - fn comments(&mut self) -> &mut Option> { - &mut self.comments + fn comments(&self) -> Option<&Comments<'a>> { + self.comments.as_ref() + } + + fn comments_mut(&mut self) -> Option<&mut Comments<'a>> { + self.comments.as_mut() } fn ann_post(&mut self, ident: Ident) { From 74e1b46ab23fe1f2deb414968340ca35e7882ae0 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Mon, 13 May 2024 10:06:27 +1000 Subject: [PATCH 2/2] Make `Comments::next` consume a comment. This avoids the need for a clone, fixing a FIXME comment. --- compiler/rustc_ast_pretty/src/pprust/state.rs | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/compiler/rustc_ast_pretty/src/pprust/state.rs b/compiler/rustc_ast_pretty/src/pprust/state.rs index 9eb552cdcdf..329c2167d55 100644 --- a/compiler/rustc_ast_pretty/src/pprust/state.rs +++ b/compiler/rustc_ast_pretty/src/pprust/state.rs @@ -55,8 +55,8 @@ impl PpAnn for NoAnn {} pub struct Comments<'a> { sm: &'a SourceMap, - comments: Vec, - current: usize, + // Stored in reverse order so we can consume them by popping. + reversed_comments: Vec, } /// Returns `None` if the first `col` chars of `s` contain a non-whitespace char. @@ -182,19 +182,17 @@ fn gather_comments(sm: &SourceMap, path: FileName, src: String) -> Vec impl<'a> Comments<'a> { pub fn new(sm: &'a SourceMap, filename: FileName, input: String) -> Comments<'a> { - let comments = gather_comments(sm, filename, input); - Comments { sm, comments, current: 0 } + let mut comments = gather_comments(sm, filename, input); + comments.reverse(); + Comments { sm, reversed_comments: comments } } fn peek(&self) -> Option<&Comment> { - self.comments.get(self.current) + self.reversed_comments.last() } - // FIXME: This shouldn't probably clone lmao fn next(&mut self) -> Option { - let cmnt = self.comments.get(self.current).cloned(); - self.current += 1; - cmnt + self.reversed_comments.pop() } fn trailing_comment(