Skip to content
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

Remove no need binds.empty? checking #22037

Merged
merged 1 commit into from
Oct 23, 2015

Conversation

kamipo
Copy link
Member

@kamipo kamipo commented Oct 23, 2015

#exec_stmt is private method and only called in #exec_query. it
means binds is provided always. No need binds.empty? checking.

`#exec_stmt` is private method and only called in `#exec_query`. it
means `binds` is provided always. No need `binds.empty?` checking.
@rails-bot
Copy link

r? @sgrif

(@rails-bot has picked a reviewer for you, use r? to override)

@sgrif
Copy link
Contributor

sgrif commented Oct 23, 2015

binds is sometimes an empty array

@kamipo
Copy link
Member Author

kamipo commented Oct 23, 2015

@sgrif exec_stmt is private method and only called by this line:

https://github.com/kamipo/rails/blob/remove_no_need_binds_empty/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb#L242

      def exec_query(sql, name = 'SQL', binds = [], prepare: false)
        if without_prepared_statement?(binds)
          result_set, affected_rows = exec_without_stmt(sql, name)
        else
          result_set, affected_rows = exec_stmt(sql, name, binds, cache_stmt: prepare)
        end
...

without_prepared_statement? is here:

https://github.com/kamipo/rails/blob/remove_no_need_binds_empty/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb#L525-L527

      def without_prepared_statement?(binds)
        !prepared_statements || binds.empty?
      end

binds is not passed to exec_stmt when it is empty array. What do you think?

@@ -417,7 +417,7 @@ def exec_stmt(sql, name, binds, cache_stmt: false)
affected_rows = stmt.affected_rows

stmt.free_result
stmt.close if binds.empty?
stmt.close if !cache_stmt
Copy link
Member Author

Choose a reason for hiding this comment

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

This is follow up to cbcdecd.
We should add !cache_stmt to this line.

@sgrif sgrif reopened this Oct 23, 2015
sgrif added a commit that referenced this pull request Oct 23, 2015
Remove no need `binds.empty?` checking
@sgrif sgrif merged commit e817196 into rails:master Oct 23, 2015
@kamipo kamipo deleted the remove_no_need_binds_empty branch October 23, 2015 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants