Skip to content

Simple formatting of some modules #24927

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.

Sign up for GitHub

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

Closed
wants to merge 8 commits into from
Closed

Simple formatting of some modules #24927

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2d3d1a8
Add a new test group, rfail-full that runs rfail tests with fulldeps.
rprichard Apr 26, 2015
46f9717
Move qquote.rs from run-fail to run-fail-fulldeps
rprichard Apr 26, 2015
99a672a
Merge branch 'master' of https://github.com/mozilla/rust
nrc May 1, 2015
aa1f9e8
Formatting librustc_resolve
nrc Apr 29, 2015
8ab684e
Formatting librustc_trans/save
nrc Apr 29, 2015
61c66aa
Formatting libflate
nrc Apr 29, 2015
26818d3
Format libgraphviz
nrc May 1, 2015
5255db7
Manual fixups for libgraphviz
nrc May 1, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions src/libflate/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,9 @@
#![feature(unique)]
#![cfg_attr(test, feature(rustc_private, rand, collections))]

#[cfg(test)] #[macro_use] extern crate log;
#[cfg(test)]
#[macro_use]
extern crate log;

extern crate libc;

Expand Down Expand Up @@ -123,7 +125,7 @@ pub fn deflate_bytes_zlib(bytes: &[u8]) -> Bytes {
deflate_bytes_internal(bytes, LZ_NORM | TDEFL_WRITE_ZLIB_HEADER)
}

fn inflate_bytes_internal(bytes: &[u8], flags: c_int) -> Result<Bytes,Error> {
fn inflate_bytes_internal(bytes: &[u8], flags: c_int) -> Result<Bytes, Error> {
unsafe {
let mut outsz: size_t = 0;
let res = tinfl_decompress_mem_to_heap(bytes.as_ptr() as *const _,
Expand All @@ -142,12 +144,12 @@ fn inflate_bytes_internal(bytes: &[u8], flags: c_int) -> Result<Bytes,Error> {
}

/// Decompress a buffer, without parsing any sort of header on the input.
pub fn inflate_bytes(bytes: &[u8]) -> Result<Bytes,Error> {
pub fn inflate_bytes(bytes: &[u8]) -> Result<Bytes, Error> {
inflate_bytes_internal(bytes, 0)
}

/// Decompress a buffer that starts with a zlib header.
pub fn inflate_bytes_zlib(bytes: &[u8]) -> Result<Bytes,Error> {
pub fn inflate_bytes_zlib(bytes: &[u8]) -> Result<Bytes, Error> {
inflate_bytes_internal(bytes, TINFL_FLAG_PARSE_ZLIB_HEADER)
}

Expand Down
105 changes: 62 additions & 43 deletions src/libgraphviz/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -432,15 +432,17 @@ pub trait Labeller<'a,N,E> {
}

impl<'a> LabelText<'a> {
pub fn label<S:IntoCow<'a, str>>(s: S) -> LabelText<'a> {
pub fn label<S: IntoCow<'a, str>>(s: S) -> LabelText<'a> {
LabelStr(s.into_cow())
}

pub fn escaped<S:IntoCow<'a, str>>(s: S) -> LabelText<'a> {
pub fn escaped<S: IntoCow<'a, str>>(s: S) -> LabelText<'a> {
EscStr(s.into_cow())
}

fn escape_char<F>(c: char, mut f: F) where F: FnMut(char) {
fn escape_char<F>(c: char, mut f: F)
where F: FnMut(char)
{
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels strange since the curly braces normally open at the end of the function signature, but here it's on it's own line. I understand why it's done, but there's a definite aesthetic dissonance.

match c {
// not escaping \\, since Graphviz escString needs to
// interpret backslashes; see EscStr above.
Expand Down Expand Up @@ -531,29 +533,40 @@ pub enum RenderOption {
}

/// Returns vec holding all the default render options.
pub fn default_options() -> Vec<RenderOption> { vec![] }
pub fn default_options() -> Vec<RenderOption> {
vec![]
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should also support the style of function as it was before.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do like having the whole fn sig and body in one line when the body really is short. Is this hard to accommodate, @nrc ? Or is this more of a philosophical thing about "bodies should be presented separately from signatures" ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit of both tbh, I do prefer function bodies always having their own line - makes it easier to read and distinguish provided from required methods. It was also the easiest thing to do.


/// Renders directed graph `g` into the writer `w` in DOT syntax.
/// (Simple wrapper around `render_opts` that passes a default set of options.)
pub fn render<'a, N:Clone+'a, E:Clone+'a, G:Labeller<'a,N,E>+GraphWalk<'a,N,E>, W:Write>(
g: &'a G,
w: &mut W) -> io::Result<()> {
pub fn render<'a,
N: Clone + 'a,
E: Clone + 'a,
G: Labeller<'a, N, E> + GraphWalk<'a, N, E>,
W: Write>
(g: &'a G,
w: &mut W)
-> io::Result<()> {
render_opts(g, w, &[])
}

/// Renders directed graph `g` into the writer `w` in DOT syntax.
/// (Main entry point for the library.)
pub fn render_opts<'a, N:Clone+'a, E:Clone+'a, G:Labeller<'a,N,E>+GraphWalk<'a,N,E>, W:Write>(
g: &'a G,
w: &mut W,
options: &[RenderOption]) -> io::Result<()>
{
fn writeln<W:Write>(w: &mut W, arg: &[&str]) -> io::Result<()> {
pub fn render_opts<'a,
N: Clone + 'a,
E: Clone + 'a,
G: Labeller<'a, N, E> + GraphWalk<'a, N, E>,
W: Write>
(g: &'a G,
w: &mut W,
options: &[RenderOption])
-> io::Result<()> {
fn writeln<W: Write>(w: &mut W, arg: &[&str]) -> io::Result<()> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to better distinguish between the function arguments and body? They blend together visually for me. My personal style is:

pub fn render_opts<
    'a,
    N: Clone+'a,
    E: Clone+'a,
    G: Labeller<'a, N, E>+GraphWalk<'a, N, E>,
    W: Write,
>(
    g: &'a G,
    w: &mut W,
    options: &[RenderOption],
) -> io::Result<()> {
    fn writeln<W: Write>(w: &mut W, arg: &[&str]) -> io::Result<()> {

To make it a little closer aligned to this, it would at least be nice if the argument list is long that the { is bumped to the next line, a la:

pub fn render_opts<'a,
                   N: Clone+'a,
                   E: Clone+'a,
                   G: Labeller<'a, N, E>+GraphWalk<'a, N, E>,
                   W: Write>
    (g: &'a G,
     w: &mut W,
     options: &[RenderOption])
     -> io::Result<()>
{
    fn writeln<W: Write>(w: &mut W, arg: &[&str]) -> io::Result<()> {

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rustfmt currently only does newline brace if there is a where clause, I would like to do it everywhere, but that is a big style change. I guess bumping it if it is a mulit-line sig is a compromise, but it seems somewhat inconsistent to me.

for &s in arg { try!(w.write_all(s.as_bytes())); }
write!(w, "\n")
}

fn indent<W:Write>(w: &mut W) -> io::Result<()> {
fn indent<W: Write>(w: &mut W) -> io::Result<()> {
w.write_all(b" ")
}

Expand Down Expand Up @@ -657,9 +670,7 @@ mod tests {
}

impl LabelledGraph {
fn new(name: &'static str,
node_labels: Trivial,
edges: Vec<Edge>) -> LabelledGraph {
fn new(name: &'static str, node_labels: Trivial, edges: Vec<Edge>) -> LabelledGraph {
LabelledGraph {
name: name,
node_labels: node_labels.to_opt_strs(),
Expand All @@ -671,7 +682,8 @@ mod tests {
impl LabelledGraphWithEscStrs {
fn new(name: &'static str,
node_labels: Trivial,
edges: Vec<Edge>) -> LabelledGraphWithEscStrs {
edges: Vec<Edge>)
-> LabelledGraphWithEscStrs {
LabelledGraphWithEscStrs {
graph: LabelledGraph::new(name, node_labels, edges)
}
Expand All @@ -695,52 +707,56 @@ mod tests {
None => LabelStr(id_name(n).name()),
}
}
fn edge_label(&'a self, e: & &'a Edge) -> LabelText<'a> {
fn edge_label(&'a self, e: &&'a Edge) -> LabelText<'a> {
LabelStr(e.label.into_cow())
}
}

impl<'a> Labeller<'a, Node, &'a Edge> for LabelledGraphWithEscStrs {
fn graph_id(&'a self) -> Id<'a> { self.graph.graph_id() }
fn node_id(&'a self, n: &Node) -> Id<'a> { self.graph.node_id(n) }
fn graph_id(&'a self) -> Id<'a> {
self.graph.graph_id()
}
fn node_id(&'a self, n: &Node) -> Id<'a> {
self.graph.node_id(n)
}
fn node_label(&'a self, n: &Node) -> LabelText<'a> {
match self.graph.node_label(n) {
LabelStr(s) | EscStr(s) => EscStr(s),
}
}
fn edge_label(&'a self, e: & &'a Edge) -> LabelText<'a> {
fn edge_label(&'a self, e: &&'a Edge) -> LabelText<'a> {
match self.graph.edge_label(e) {
LabelStr(s) | EscStr(s) => EscStr(s),
}
}
}

impl<'a> GraphWalk<'a, Node, &'a Edge> for LabelledGraph {
fn nodes(&'a self) -> Nodes<'a,Node> {
fn nodes(&'a self) -> Nodes<'a, Node> {
(0..self.node_labels.len()).collect()
}
fn edges(&'a self) -> Edges<'a,&'a Edge> {
fn edges(&'a self) -> Edges<'a, &'a Edge> {
self.edges.iter().collect()
}
fn source(&'a self, edge: & &'a Edge) -> Node {
fn source(&'a self, edge: &&'a Edge) -> Node {
edge.from
}
fn target(&'a self, edge: & &'a Edge) -> Node {
fn target(&'a self, edge: &&'a Edge) -> Node {
edge.to
}
}

impl<'a> GraphWalk<'a, Node, &'a Edge> for LabelledGraphWithEscStrs {
fn nodes(&'a self) -> Nodes<'a,Node> {
fn nodes(&'a self) -> Nodes<'a, Node> {
self.graph.nodes()
}
fn edges(&'a self) -> Edges<'a,&'a Edge> {
fn edges(&'a self) -> Edges<'a, &'a Edge> {
self.graph.edges()
}
fn source(&'a self, edge: & &'a Edge) -> Node {
fn source(&'a self, edge: &&'a Edge) -> Node {
edge.from
}
fn target(&'a self, edge: & &'a Edge) -> Node {
fn target(&'a self, edge: &&'a Edge) -> Node {
edge.to
}
}
Expand Down Expand Up @@ -781,8 +797,7 @@ r#"digraph single_node {
#[test]
fn single_edge() {
let labels : Trivial = UnlabelledNodes(2);
let result = test_input(LabelledGraph::new("single_edge", labels,
vec!(edge(0, 1, "E"))));
let result = test_input(LabelledGraph::new("single_edge", labels, vec!(edge(0, 1, "E"))));
assert_eq!(result.unwrap(),
r#"digraph single_edge {
N0[label="N0"];
Expand All @@ -795,7 +810,8 @@ r#"digraph single_edge {
#[test]
fn test_some_labelled() {
let labels : Trivial = SomeNodesLabelled(vec![Some("A"), None]);
let result = test_input(LabelledGraph::new("test_some_labelled", labels,
let result = test_input(LabelledGraph::new("test_some_labelled",
labels,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a rule that "either all parameters must be on their respective line, or they must share one"?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I'm fine with such a rule, personally.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

vec![edge(0, 1, "A-1")]));
assert_eq!(result.unwrap(),
r#"digraph test_some_labelled {
Expand All @@ -809,8 +825,7 @@ r#"digraph test_some_labelled {
#[test]
fn single_cyclic_node() {
let labels : Trivial = UnlabelledNodes(1);
let r = test_input(LabelledGraph::new("single_cyclic_node", labels,
vec!(edge(0, 0, "E"))));
let r = test_input(LabelledGraph::new("single_cyclic_node", labels, vec!(edge(0, 0, "E"))));
assert_eq!(r.unwrap(),
r#"digraph single_cyclic_node {
N0[label="N0"];
Expand All @@ -822,10 +837,12 @@ r#"digraph single_cyclic_node {
#[test]
fn hasse_diagram() {
let labels = AllNodesLabelled(vec!("{x,y}", "{x}", "{y}", "{}"));
let r = test_input(LabelledGraph::new(
"hasse_diagram", labels,
vec!(edge(0, 1, ""), edge(0, 2, ""),
edge(1, 3, ""), edge(2, 3, ""))));
let r = test_input(LabelledGraph::new("hasse_diagram",
labels,
vec!(edge(0, 1, ""),
edge(0, 2, ""),
edge(1, 3, ""),
edge(2, 3, ""))));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. I would have maybe hoped that we would support this:

+        let r = test_input(LabelledGraph::new(
+            "hasse_diagram",
+            labels,
+            vec!(edge(0, 1, ""), edge(0, 2, ""), edge(1, 3, ""), edge(2, 3, ""))));

That is, I think the idiom of putting a line break before the first argument to avoid the heavy indentation of the argument list is a pretty common idiom for us, no?

But maybe it is too hard for rustfmt to decide whether to adopt that strategy or not, since it is often subjective whether to employ it or not.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I'm not sure how to weight that decision - the heuristic I would use would try hanging indent if it means we can keep each argument on one line, which would address this case, I'm not sure if that is a good general rule.

I actually prefer the first layout and would use that if handwriting the code. But I have a stong aversion to hanging indents, I can see why you might prefer the latter.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So on further reflection, how about: Try hanging indent if it means we can keep all the arguments on the same line?

(It won't address this case, but that is okay with me.)

Of course, for all I know my suggestion is what rustfmt already does ...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My 2 cents: I find it depends on the case, sometimes I prefer hanging indent, sometimes not, depending on various concerns. I think it'd be good to preserve what the author chose. I really dislike when all the code is bunched up against the right margin though, so I tend to prefer hanging idents if that's the only way to get "full" utilization.

assert_eq!(r.unwrap(),
r#"digraph hasse_diagram {
N0[label="{x,y}"];
Expand Down Expand Up @@ -856,10 +873,12 @@ r#"digraph hasse_diagram {

let mut writer = Vec::new();

let g = LabelledGraphWithEscStrs::new(
"syntax_tree", labels,
vec!(edge(0, 1, "then"), edge(0, 2, "else"),
edge(1, 3, ";"), edge(2, 3, ";" )));
let g = LabelledGraphWithEscStrs::new("syntax_tree",
labels,
vec!(edge(0, 1, "then"),
edge(0, 2, "else"),
edge(1, 3, ";"),
edge(2, 3, ";" )));

render(&g, &mut writer).unwrap();
let mut r = String::new();
Expand Down
Loading