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

bind_rows() rejects POSIXlt columns #1875

Merged
merged 2 commits into from Jun 1, 2016
Merged

Conversation

@krlmlr
Copy link
Member

@krlmlr krlmlr commented Jun 1, 2016

instead of segfaulting.

Fixes #1789.

Also fixes another instance with the same pattern, but I'm not sure if this is strictly necessary.

NEWS line:

* `bind_rows()` cleanly aborts if one of the data frames contains a `POSIXlt` column
  (#1789, #1875, @krlmlr).
@codecov-io
Copy link

@codecov-io codecov-io commented Jun 1, 2016

Current coverage is 20.00%

Merging #1875 into master will decrease coverage by <.01%

@@             master      #1875   diff @@
==========================================
  Files           188        188          
  Lines          7422       7425     +3   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits           1485       1485          
- Misses         5937       5940     +3   
  Partials          0          0          

Powered by Codecov. Last updated by a1c4faf...d978987

@hadley
Copy link
Member

@hadley hadley commented Jun 1, 2016

Does the logic in order_visitor() look right to you?

                    case VECSXP:
                    {
                        if( Rf_inherits( vec, "data.frame" ) ){
                            return new OrderVisitorDataFrame<true>( vec ) ;
                        }
                        break ;
                    }

I think that will fall through to the return 0. I think the break should be removed, since we don't have a general list orderer (and POSIXlt doesn't need to be a special case)

@krlmlr
Copy link
Member Author

@krlmlr krlmlr commented Jun 1, 2016

No, the logic looks odd. I'll submit a pull.

Fall-through in switch() blocks (removing the break) is an anti-pattern and may even induce a compiler warning.

@hadley
Copy link
Member

@hadley hadley commented Jun 1, 2016

@krlmlr good to know - better to be explicit anyway

@krlmlr
Copy link
Member Author

@krlmlr krlmlr commented Jun 1, 2016

In clang: -Wimplicit-fallthrough (http://stackoverflow.com/a/27965827/946850), work in progress for gcc.

@hadley hadley merged commit 265db7b into tidyverse:master Jun 1, 2016
1 of 3 checks passed
@hadley
Copy link
Member

@hadley hadley commented Jun 1, 2016

Thanks!

@krlmlr krlmlr deleted the feature/bind-posixlt branch Jun 1, 2016
@krlmlr krlmlr mentioned this pull request Jun 4, 2016
@lock
Copy link

@lock lock bot commented Jan 18, 2019

This old issue has been automatically locked. If you believe you have found a related problem, please file a new issue (with reprex) and link to this issue. https://reprex.tidyverse.org/

@lock lock bot locked and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants