Add test build with frozen strings #374

miry · 2018-08-13T18:23:01Z

In ruby 3 option --enable-frozen-string-literal will be enabled by
default.

coveralls · 2018-08-13T18:33:05Z

Coverage decreased (-4.2%) to 94.557% when pulling 18ea47a on miry:enable-frozen-builds into e89f6ac on rubyzip:master.

bdewater · 2018-08-21T19:10:28Z

lib/zip/inflater.rb

@@ -12,7 +12,7 @@ def sysread(number_of_bytes = nil, buf = '')
      readEverything = number_of_bytes.nil?
      while readEverything || @output_buffer.bytesize < number_of_bytes
        break if internal_input_finished?
-        @output_buffer << internal_produce_input(buf)
+        @output_buffer += internal_produce_input(buf)


This will create a new and appended string for every loop, not very efficient. Why not change the method signature to def sysread(number_of_bytes = nil, buf = String.new('')) for a mutable string we can append to?

@bdewater Fixed in this method signature and other places where string initialized.

bdewater · 2018-08-21T19:14:13Z

lib/zip/inflater.rb

@@ -38,7 +38,7 @@ def input_finished?

    private

-    def internal_produce_input(buf = '')
+    def internal_produce_input(buf = String.new)


String.new without an argument will default to ASCII-8BIT encoding while '' is UTF-8. If you change this to String.new('') the encoding will remain UTF-8.

@bdewater good to know. fixed.

bdewater · 2018-08-23T19:28:17Z

lib/zip/ioextras/abstract_input_stream.rb

@@ -11,26 +11,27 @@ def initialize
        super
        @lineno        = 0
        @pos           = 0
-        @output_buffer = ''
+        @output_buffer = String.new


Same encoding issue.

bdewater · 2018-08-23T19:28:44Z

lib/zip/ioextras/abstract_input_stream.rb

      end

      attr_accessor :lineno
      attr_reader :pos

      def read(number_of_bytes = nil, buf = '')
+        buffer = buf.dup


Why don't you make buf a mutable String.new('') instead?

bdewater · 2018-08-23T19:29:32Z

lib/zip/ioextras/abstract_input_stream.rb

@@ -76,7 +77,7 @@ def gets(a_sep_string = $/, number_of_bytes = nil)
        while (match_index = @output_buffer.index(a_sep_string, buffer_index)).nil? && !over_limit
          buffer_index = [buffer_index, @output_buffer.bytesize - a_sep_string.bytesize].max
          return @output_buffer.empty? ? nil : flush if input_finished?
-          @output_buffer << produce_input
+          @output_buffer += produce_input


Same appending issue as mentioned above.

@bdewater thanks. Fixed.

bdewater · 2018-08-23T19:29:41Z

lib/zip/ioextras/abstract_input_stream.rb

@@ -90,7 +91,7 @@ def ungetc(byte)

      def flush
        ret_val        = @output_buffer
-        @output_buffer = ''
+        @output_buffer = String.new


Same encoding issue.

@bdewater fixed

bdewater · 2018-08-23T19:34:11Z

.travis.yml

@@ -17,6 +17,9 @@ matrix:
    jdk: openjdk7
  - rvm: jruby-head
    jdk: oraclejdk8
+  - rvm: ruby-head
+    env:
+      - RUBYOPT="--enable-frozen-string-literal"


Why not put the # frozen_string_literal: true magic comment a top of every file so that every Ruby 2.3+ user can reap the benefits? I doubt a lot of people can their Ruby app with this RUBYOPT flag since not every gem is ready for this yet.

@bdewater I don't think to update all files in gem, without automatic rubocop/lint checking is useful. It would inconsistent file to file.

I wanted to check that gem is ready, even without force people who uses gem to fix code. I came to this PR exactly, because enabled in all file of a project # frozen_string_literal: true. I don't think I fixed all problems. But at least it would be in the matrix.

This record is optional. And it is included ruby head tests without any env.

https://travis-ci.org/rubyzip/rubyzip/builds/415587058

bdewater · 2018-08-23T19:35:17Z

test/filesystem/file_nonmutating_test.rb

@@ -55,7 +55,7 @@ def test_open_read
                   f.readline.chomp)
    end
    assert(blockCalled)
-    @zip_file.dir.chdir '/'
+    @zip_file.dir.chdir String.new('/')


This change is not needed.

miry · 2018-08-23T20:29:29Z

@bdewater thanks for your comments. I updated PR.

In ruby 3 option `--enable-frozen-string-literal` will be enabled by default.

…flater.

miry · 2018-10-12T18:31:57Z

closing because there were no action for last 30 days.

miry force-pushed the enable-frozen-builds branch from 903f052 to b128e0d Compare August 13, 2018 18:32

bdewater reviewed Aug 23, 2018

View reviewed changes

miry added 7 commits August 23, 2018 22:34

Add test build with frozen strings

9a6c79d

In ruby 3 option `--enable-frozen-string-literal` will be enabled by default.

Updated tests to use dynamiz string

e7f9b12

Run test against ruby head with default ruby options.

440592a

Use string as default option for buf.

2e16e2c

Use mutable string for output buffer variables in input stream and in…

bf6ca3d

…flater.

Applied comments related to encdoing and useless changes.

8f2d39e

Revert duplication of buffer

18ea47a

miry force-pushed the enable-frozen-builds branch from 2699221 to 18ea47a Compare August 23, 2018 20:35

miry closed this Oct 12, 2018

This was referenced Mar 22, 2019

fixed FrozenError caused by frozen-string-literal #390

Merged

Update supported ruby versions #392

Merged

Add test build with frozen strings #374

Add test build with frozen strings #374

Uh oh!

Conversation

miry commented Aug 13, 2018

Uh oh!

coveralls commented Aug 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miry Aug 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miry commented Aug 23, 2018

Uh oh!

miry commented Oct 12, 2018

Uh oh!

Uh oh!

coveralls commented Aug 13, 2018 •

edited

Loading

miry Aug 23, 2018 •

edited

Loading