Skip to content

ARROW-782: [C++] API cleanup, change public member access in DataType classes to functions, use class instead of struct#520

Closed
wesm wants to merge 5 commits intoapache:masterfrom
wesm:ARROW-782
Closed

ARROW-782: [C++] API cleanup, change public member access in DataType classes to functions, use class instead of struct#520
wesm wants to merge 5 commits intoapache:masterfrom
wesm:ARROW-782

Conversation

@wesm
Copy link
Copy Markdown
Member

@wesm wesm commented Apr 10, 2017

Breaking APIs isn't ideal, but this one is fairly long overdue. The DataType classes are more than passive data carriers, and Google's C++ guide recommends using class instead of struct for this. That means we should put members in protected or private scope, and access them.

I also renamed a couple of things to help with code clarity

  • DataType::type is now DataType::id()
  • Array::type_enum is not Array::type_id

wesm added 3 commits April 9, 2017 21:13
…ss instead of struct

Change-Id: Id0551429b786c747af898ddb72ba03bfbd6fc91b
Change-Id: I4e1982a30f854df8ab4129e27f5d9db8d467fa91
Change-Id: Id19bcd2190215c9950abc99b29a47d3e729ca747
Change-Id: Iaf48f56c361fd4a14b4a4964f15c4276f8a8f634
@wesm
Copy link
Copy Markdown
Member Author

wesm commented Apr 10, 2017

@kou I changed std::shared_ptr<DataType>* to DataType*, it didn't seem like the object created has explicit ownership of this object but correct me if I'm wrong about that

@kou
Copy link
Copy Markdown
Member

kou commented Apr 10, 2017

I think that it may cause GC related crash in bindings. But I'm not sure without trying it. So it's OK the change. If the change has a problem, I just revert the change.

@wesm
Copy link
Copy Markdown
Member Author

wesm commented Apr 10, 2017

Ok, the problem now is that the type() method returns a new shared_ptr; I will let you take care of it

Change-Id: I8fd7b3123e9b85a1cabccb0913e36ec99acd5cdb
@wesm
Copy link
Copy Markdown
Member Author

wesm commented Apr 10, 2017

+1

@asfgit asfgit closed this in 793f4e0 Apr 10, 2017
@wesm wesm deleted the ARROW-782 branch April 10, 2017 13:25
pribor pushed a commit to GlobalWebIndex/arrow that referenced this pull request Oct 24, 2025
Fixes apacheGH-520.

The configuration file is borrowed from apache/arrow.
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.

2 participants