You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2020/05/31 23:19:54 UTC
[GitHub] [arrow] wesm opened a new pull request #7310: ARROW-6052: [C++] Split up arrow/array.h/cc into multiple files under arrow/array/, move ArrayData to separate header
wesm opened a new pull request #7310:
URL: https://github.com/apache/arrow/pull/7310
This allows some files that only need ArrayData or just Array to not include a bunch of unnecessary header code.
I did a modest amount of IWYU cleaning, there are plenty of still usages of "arrow/array.h" that might be better replaced by more specific headers.
As part of this patch I disabled `build/include_what_you_use` in cpplint per ARROW-8994. If we're going to spend time doing IWYU cleaning (which IMHO is overall worthwhile) we should rely on its output rather than having IWYU checks by two different tools.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wesm closed pull request #7310: ARROW-6052: [C++] Split up arrow/array.h/cc into multiple files under arrow/array/, move ArrayData to separate header, make ArrayData::dictionary ArrayData
Posted by GitBox <gi...@apache.org>.
wesm closed pull request #7310:
URL: https://github.com/apache/arrow/pull/7310
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wesm commented on a change in pull request #7310: ARROW-6052: [C++] Split up arrow/array.h/cc into multiple files under arrow/array/, move ArrayData to separate header, make ArrayData::dictionary ArrayData
Posted by GitBox <gi...@apache.org>.
wesm commented on a change in pull request #7310:
URL: https://github.com/apache/arrow/pull/7310#discussion_r433855919
##########
File path: cpp/src/arrow/compute/exec.h
##########
@@ -36,6 +36,7 @@
namespace arrow {
class Array;
+class BooleanArray;
Review comment:
Note: IWYU unfortunately doesn't recognize forward declarations (https://github.com/include-what-you-use/include-what-you-use/issues/530) but we can add mappings to build-support/iwyu/mappings/arrow-misc.imp to attribute symbols to type_fwd.h so it won't complain about them. I'll fill up these mappings incrementally to help
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wesm commented on a change in pull request #7310: ARROW-6052: [C++] Split up arrow/array.h/cc into multiple files under arrow/array/, move ArrayData to separate header, make ArrayData::dictionary ArrayData
Posted by GitBox <gi...@apache.org>.
wesm commented on a change in pull request #7310:
URL: https://github.com/apache/arrow/pull/7310#discussion_r433907715
##########
File path: cpp/src/arrow/compute/exec.h
##########
@@ -36,6 +36,7 @@
namespace arrow {
class Array;
+class BooleanArray;
Review comment:
I made some improvements to our IWYU scripts to not complain so much about forward declarations, it should help a lot
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wesm commented on pull request #7310: ARROW-6052: [C++] Split up arrow/array.h/cc into multiple files under arrow/array/, move ArrayData to separate header, make ArrayData::dictionary ArrayData
Posted by GitBox <gi...@apache.org>.
wesm commented on pull request #7310:
URL: https://github.com/apache/arrow/pull/7310#issuecomment-637224951
Probably be good to merge this soon to limit rebase headaches
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #7310: ARROW-6052: [C++] Split up arrow/array.h/cc into multiple files under arrow/array/, move ArrayData to separate header, make ArrayData::dictionary ArrayData
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #7310:
URL: https://github.com/apache/arrow/pull/7310#discussion_r433799924
##########
File path: cpp/src/arrow/table.h
##########
@@ -24,13 +24,18 @@
#include <vector>
#include "arrow/record_batch.h"
+#include "arrow/result.h"
+#include "arrow/status.h"
#include "arrow/type.h"
-#include "arrow/type_fwd.h"
#include "arrow/util/macros.h"
#include "arrow/util/visibility.h"
namespace arrow {
+class Array;
+class KeyValueMetadata;
+class MemoryPool;
Review comment:
Include "type_fwd.h" instead?
##########
File path: cpp/src/arrow/compute/exec.h
##########
@@ -36,6 +36,7 @@
namespace arrow {
class Array;
+class BooleanArray;
Review comment:
Shouldn't this be in `type_fwd.h`? You could remove all those explicit forward declarations...
##########
File path: cpp/src/arrow/table.cc
##########
@@ -37,6 +39,10 @@
namespace arrow {
+class KeyValueMetadata;
+class MemoryPool;
+struct ArrayData;
Review comment:
Include "type_fwd.h" instead?
##########
File path: cpp/src/arrow/util/hashing.h
##########
@@ -31,25 +31,29 @@
#include <utility>
#include <vector>
-#include "arrow/array.h"
-#include "arrow/buffer.h"
-#include "arrow/builder.h"
+#include "arrow/array/builder_binary.h"
+#include "arrow/buffer_builder.h"
+#include "arrow/result.h"
#include "arrow/status.h"
-#include "arrow/type.h"
#include "arrow/type_traits.h"
#include "arrow/util/bit_util.h"
-#include "arrow/util/checked_cast.h"
#include "arrow/util/logging.h"
#include "arrow/util/macros.h"
-#include "arrow/util/string_view.h"
+#include "arrow/util/ubsan.h"
#define XXH_INLINE_ALL
#define XXH_PRIVATE_API
#define XXH_NAMESPACE arrow_hashing_
-#include "arrow/vendored/xxhash.h"
+#include "arrow/vendored/xxhash.h" // IWYU pragma: keep
namespace arrow {
+
+class BooleanType;
+class Buffer;
+class LargeBinaryType;
+class MemoryPool;
Review comment:
Include "type_fwd.h" instead?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wesm commented on pull request #7310: ARROW-6052: [C++] Split up arrow/array.h/cc into multiple files under arrow/array/, move ArrayData to separate header, make ArrayData::dictionary ArrayData
Posted by GitBox <gi...@apache.org>.
wesm commented on pull request #7310:
URL: https://github.com/apache/arrow/pull/7310#issuecomment-637493442
I'll rebase and address the comments, and then merge later today.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #7310: ARROW-6052: [C++] Split up arrow/array.h/cc into multiple files under arrow/array/, move ArrayData to separate header, make ArrayData::dictionary ArrayData
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #7310:
URL: https://github.com/apache/arrow/pull/7310#issuecomment-636547348
https://issues.apache.org/jira/browse/ARROW-6052
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wesm commented on pull request #7310: ARROW-6052: [C++] Split up arrow/array.h/cc into multiple files under arrow/array/, move ArrayData to separate header, make ArrayData::dictionary ArrayData
Posted by GitBox <gi...@apache.org>.
wesm commented on pull request #7310:
URL: https://github.com/apache/arrow/pull/7310#issuecomment-637595364
Appveyor looks fine: https://ci.appveyor.com/project/wesm/arrow/builds/33267645. I'm going to go ahead and merge this
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org