Add clippy check to travis #986

chrisduerr · 2018-01-02T22:23:44Z

This commit adds clippy as a required step of the build process. To make
this possible, all existing clippy issues have been resolved.

I think making clippy part of the travis CI run would be a great step,
but if this is not desired, I can also take that part out of this and
add just the fixes for all current clippy issues.

jwilm

Thanks for the PR! I'd like to land this and run clippy regularly, but not have travis break the build if clippy fails. We would still want it to be obvious on the status that clippy failed, though.

I think one way we could work this is to have travis open a second status issue like "clippy" for each commit it builds and post the clippy status separately from tests. I've asked #travis people about this before, and apparently there's no built-in support for it. We would have to wrap GitHub API ourselves.

Left some comments about changes before I would land.

jwilm · 2018-01-03T16:50:07Z

src/ansi.rs

-                        buf.push_str(&format!("{:?},", *item as char));
-                    }
-                    buf.push_str("],");
+        #[inline]


We probably don't need inline here. I honestly can't remember why this was ever a macro...

jwilm · 2018-01-03T17:22:19Z

src/logging.rs

@@ -28,6 +28,7 @@ pub struct Logger<T> {
 }

 impl<T: Send + io::Write> Logger<T> {
+    #[cfg_attr(feature = "clippy", allow(new_ret_no_self))]


Let's add a comment here pointing to the issue tracking this broken lint: rust-lang/rust-clippy#734

jwilm · 2018-01-03T17:23:20Z

src/renderer/mod.rs

@@ -1125,7 +1125,7 @@ impl ShaderProgram {
            gl::GetShaderiv(shader, gl::COMPILE_STATUS, &mut success);
        }

-        if success == (gl::TRUE as GLint) {
+        if success == i32::from(gl::TRUE) {


This should probably be GLint::from rather than i32::from.

jwilm · 2018-01-03T17:23:41Z

src/selection.rs

@@ -136,25 +136,22 @@ impl Selection {
            Selection::Simple { ref mut region } => {
                region.end = Anchor::new(location, side);
            },
-            Selection::Semantic { ref mut region, .. } => {
+            Selection::Semantic { ref mut region, .. } | Selection::Lines { ref mut region, .. } => {


Break line after pipe

jwilm · 2018-01-03T17:27:52Z

.travis.yml


 script:
-  - cargo test --no-default-features
+  - ./build.sh


Let's try and keep all the shell commands in the script section rather than adding build.sh. The build.sh script can be

- if [[ $VERSION == "nightly" ]]; then cargo test --no-default-features --features "clippy"; fi - if [[ $VERSION != "nightly" ]]; then cargo test --no-default-features; fi

jwilm · 2018-01-03T17:28:37Z

src/ansi.rs

@@ -360,6 +360,9 @@ pub enum CursorStyle {

    /// Cursor is a vertical bar `⎸`
    Beam,
+
+    /// Cursor is a box like `☐`
+    Box,


This looks new in this PR. Can you pull it out into a separate patch? Small nit also -- it would be nice to name this Hollow or Rect or something other than Box.

Or maybe HollowBlock would be best

jwilm · 2018-01-03T17:30:24Z

src/term/mod.rs

@@ -988,6 +990,11 @@ impl Term {
    ) -> RenderableCellsIter {
        let selection = selection.and_then(|s| s.to_span(self))
            .map(|span| span.to_range());
+        let cursor = if window_focused {


Unrelated to this PR

These changes were made to reduce the amount of parameters of the method below clippy's threshold.

jwilm · 2018-01-03T17:30:54Z

src/tty.rs

@@ -70,8 +70,8 @@ fn openpty(rows: u8, cols: u8) -> (c_int, c_int) {
    let mut slave: c_int = 0;

    let win = winsize {
-        ws_row: rows as libc::c_ushort,
-        ws_col: cols as libc::c_ushort,
+        ws_row: u16::from(rows),


c_ushort::from

jwilm · 2018-01-03T17:31:06Z

src/tty.rs

@@ -93,8 +93,8 @@ fn openpty(rows: u8, cols: u8) -> (c_int, c_int) {
    let mut slave: c_int = 0;

    let mut win = winsize {
-        ws_row: rows as libc::c_ushort,
-        ws_col: cols as libc::c_ushort,
+        ws_row: u16::from(rows),


c_ushort::from

jwilm · 2018-01-03T17:31:34Z

src/tty.rs

@@ -113,6 +113,7 @@ fn openpty(rows: u8, cols: u8) -> (c_int, c_int) {
 /// Really only needed on BSD, but should be fine elsewhere
 fn set_controlling_terminal(fd: c_int) {
    let res = unsafe {
+        #[allow(clippy)]


What didn't clippy like about this? It would be nice to just disable a single lint rather than all of clippy for this line.

This was a problem with multiple platforms. Because TIOSCTTY has a different type on macos and linux. So the conversion fails on clippy because it's either an unnecessary conversion, or an incorrect type.

I've tried to use if cfg!(target_os = "macos") but I wasn't able to make it work. Clippy still complained about the branch that shouldn't be reached on linux.

chrisduerr · 2018-01-03T19:23:52Z

Rebased this on top of master, should be good now when travis doesn't complain.

jwilm

Let's fix the CursorStyle::Box naming and then ship this

jwilm · 2018-01-05T17:14:25Z

src/ansi.rs

@@ -360,6 +360,9 @@ pub enum CursorStyle {

    /// Cursor is a vertical bar `⎸`
    Beam,
+
+    /// Cursor is a box like `☐`
+    Box,


Or maybe HollowBlock would be best

jwilm · 2018-01-05T17:17:14Z

src/tty.rs

@@ -113,7 +113,10 @@ fn openpty(rows: u8, cols: u8) -> (c_int, c_int) {
 /// Really only needed on BSD, but should be fine elsewhere
 fn set_controlling_terminal(fd: c_int) {
    let res = unsafe {
-        libc::ioctl(fd, TIOCSCTTY as _, 0)
+        // Cross platform issue because on linux this is u64 as u64 (does nothing)
+        // But on macos this is u32 as u64, asking for u64::from(u32)


Thanks for the comment!

This commit adds clippy as a required step of the build process. To make this possible, all existing clippy issues have been resolved. I think making clippy part of the travis CI run would be a great step, but if this is not desired, I can also take that part out of this and add just the fixes for all current clippy issues.

Fix all new issues introduced with the latest master.

There were some macos-specific warnings that have been fixed. The travis configuration also has been altered in an attempt to make clippy run only for nightly builds.

Added a custom build matrix so when `VERSION="nightly"` the nightly compiler is used and when `VERSION="stable"` the stable compiler is used.

Fixed a few small issues, reworked the travis configuration and added comments to the allow(_) parts.

Now it should properly make all nightly builds optional.

Changed from `c_ushort` to `libc::c_ushort`.

Rebased branch on top of master and fixed all new clippy warnings.

jwilm requested changes Jan 3, 2018

View reviewed changes

chrisduerr force-pushed the clippy branch from 4bffcec to 3789df4 Compare January 3, 2018 19:22

chrisduerr force-pushed the clippy branch from fbe2041 to cd90d10 Compare January 5, 2018 03:34

jwilm requested changes Jan 5, 2018

View reviewed changes

chrisduerr added 18 commits January 6, 2018 01:50

Rebase on top of master

9fd6f3c

Fix all new issues introduced with the latest master.

Fix macos warnings and change travis config

0336200

There were some macos-specific warnings that have been fixed. The travis configuration also has been altered in an attempt to make clippy run only for nightly builds.

Move build script to root directory

66bf65a

Change build script perms to executable

fb7f597

Change build.sh shebang to proper bash path

a67b9e8

Use rustup to run clippy with nightly

2ca7b2d

Add custom build matrix

67682d5

Added a custom build matrix so when `VERSION="nightly"` the nightly compiler is used and when `VERSION="stable"` the stable compiler is used.

Take exclude instead of include approach

414dadc

Remove identical conversion clippy warn

0dcaab0

Ignore multi-platform clippy issue

5edd173

Address feedback in PR

a36763d

Fixed a few small issues, reworked the travis configuration and added comments to the allow(_) parts.

Fix typo in travis config

557d1e8

Now it should properly make all nightly builds optional.

Remove nightly without clippy from travis config

875daa9

Fix undeclared type or module on macos

d05518e

Changed from `c_ushort` to `libc::c_ushort`.

Rebase on top of master

602cbdd

Rebased branch on top of master and fixed all new clippy warnings.

Rename the Box cursor to HollowBox

1473f60

Rename HollowBox to HollowBlock

98f71b3

chrisduerr force-pushed the clippy branch from 70f3a0b to 98f71b3 Compare January 6, 2018 00:53

chrisduerr merged commit 2920cbe into alacritty:master Jan 6, 2018

chrisduerr deleted the clippy branch January 7, 2018 16:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add clippy check to travis #986

Add clippy check to travis #986

chrisduerr commented Jan 2, 2018

jwilm left a comment

jwilm Jan 3, 2018

jwilm Jan 3, 2018

jwilm Jan 3, 2018

jwilm Jan 3, 2018

jwilm Jan 3, 2018

jwilm Jan 3, 2018

jwilm Jan 5, 2018

jwilm Jan 3, 2018

chrisduerr Jan 3, 2018

jwilm Jan 3, 2018

jwilm Jan 3, 2018

jwilm Jan 3, 2018

chrisduerr Jan 3, 2018

chrisduerr Jan 3, 2018

chrisduerr commented Jan 3, 2018 •

edited

Loading

jwilm left a comment

jwilm Jan 5, 2018

jwilm Jan 5, 2018

Add clippy check to travis #986

Add clippy check to travis #986

Conversation

chrisduerr commented Jan 2, 2018

jwilm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisduerr commented Jan 3, 2018 • edited Loading

jwilm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisduerr commented Jan 3, 2018 •

edited

Loading