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/16 15:02:44 UTC

[GitHub] [arrow] hurricane1026 opened a new pull request #7202: fix compilation for Wunused-paraemter flag

hurricane1026 opened a new pull request #7202:
URL: https://github.com/apache/arrow/pull/7202


   add a compiler hint to tell the compiler that the unused-parameter is by default.


----------------------------------------------------------------
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] hurricane1026 commented on a change in pull request #7202: ARROW-8825: [C++] Fix compilation for Wunused-paraemter flag

Posted by GitBox <gi...@apache.org>.
hurricane1026 commented on a change in pull request #7202:
URL: https://github.com/apache/arrow/pull/7202#discussion_r426291038



##########
File path: cpp/src/arrow/array.h
##########
@@ -798,7 +798,12 @@ class ARROW_EXPORT FixedSizeListArray : public Array {
     i += data_->offset;
     return static_cast<int32_t>(list_size_ * i);
   }
+#ifdef _MSC_VER
   int32_t value_length(int64_t i = 0) const { return list_size_; }
+#else
+  int32_t value_length(__attribute__((unused)) int64_t i = 0) const { return list_size_; }

Review comment:
       emmm...
   because arrow-cpp employ a very not-strictly compiling flags setting, and got too many warnings in compiling process.
   Then I use -isystem arrow_src instead of -I arrow_src in my makefile.
   So, if you guys didn't think unused-parameter or other warnings was big thing, ignoring my PR is ok.
   




----------------------------------------------------------------
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] hurricane1026 closed pull request #7202: fix compilation for Wunused-paraemter flag

Posted by GitBox <gi...@apache.org>.
hurricane1026 closed pull request #7202:
URL: https://github.com/apache/arrow/pull/7202


   


----------------------------------------------------------------
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 #7202: ARROW-8825:[C++] fix compilation for Wunused-paraemter flag

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #7202:
URL: https://github.com/apache/arrow/pull/7202#issuecomment-629680642


   https://issues.apache.org/jira/browse/ARROW-8825


----------------------------------------------------------------
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] kou commented on a change in pull request #7202: ARROW-8825: [C++] Fix compilation for Wunused-paraemter flag

Posted by GitBox <gi...@apache.org>.
kou commented on a change in pull request #7202:
URL: https://github.com/apache/arrow/pull/7202#discussion_r426192648



##########
File path: cpp/src/arrow/array.h
##########
@@ -798,7 +798,12 @@ class ARROW_EXPORT FixedSizeListArray : public Array {
     i += data_->offset;
     return static_cast<int32_t>(list_size_ * i);
   }
+#ifdef _MSC_VER
   int32_t value_length(int64_t i = 0) const { return list_size_; }
+#else
+  int32_t value_length(__attribute__((unused)) int64_t i = 0) const { return list_size_; }

Review comment:
       Can we use the `ARROW_ARG_UNUSED()` macro here?




----------------------------------------------------------------
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 #7202: fix compilation for Wunused-paraemter flag

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #7202:
URL: https://github.com/apache/arrow/pull/7202#issuecomment-629661941


   <!--
     Licensed to the Apache Software Foundation (ASF) under one
     or more contributor license agreements.  See the NOTICE file
     distributed with this work for additional information
     regarding copyright ownership.  The ASF licenses this file
     to you under the Apache License, Version 2.0 (the
     "License"); you may not use this file except in compliance
     with the License.  You may obtain a copy of the License at
   
       http://www.apache.org/licenses/LICENSE-2.0
   
     Unless required by applicable law or agreed to in writing,
     software distributed under the License is distributed on an
     "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
     KIND, either express or implied.  See the License for the
     specific language governing permissions and limitations
     under the License.
   -->
   
   Thanks for opening a pull request!
   
   Could you open an issue for this pull request on JIRA?
   https://issues.apache.org/jira/browse/ARROW
   
   Then could you also rename pull request title in the following format?
   
       ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}
   
   See also:
   
     * [Other pull requests](https://github.com/apache/arrow/pulls/)
     * [Contribution Guidelines - How to contribute patches](https://arrow.apache.org/docs/developers/contributing.html#how-to-contribute-patches)
   


----------------------------------------------------------------
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 #7202: ARROW-8825: [C++] Fix compilation for Wunused-paraemter flag

Posted by GitBox <gi...@apache.org>.
wesm closed pull request #7202:
URL: https://github.com/apache/arrow/pull/7202


   


----------------------------------------------------------------
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 #7202: ARROW-8825: [C++] Fix compilation for Wunused-paraemter flag

Posted by GitBox <gi...@apache.org>.
wesm commented on pull request #7202:
URL: https://github.com/apache/arrow/pull/7202#issuecomment-639908201


   This needs to rebased so I'll open a new PR


----------------------------------------------------------------
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] hurricane1026 commented on a change in pull request #7202: ARROW-8825: [C++] Fix compilation for Wunused-paraemter flag

Posted by GitBox <gi...@apache.org>.
hurricane1026 commented on a change in pull request #7202:
URL: https://github.com/apache/arrow/pull/7202#discussion_r426266743



##########
File path: cpp/src/arrow/array.h
##########
@@ -798,7 +798,12 @@ class ARROW_EXPORT FixedSizeListArray : public Array {
     i += data_->offset;
     return static_cast<int32_t>(list_size_ * i);
   }
+#ifdef _MSC_VER
   int32_t value_length(int64_t i = 0) const { return list_size_; }
+#else
+  int32_t value_length(__attribute__((unused)) int64_t i = 0) const { return list_size_; }

Review comment:
       I was not very clear about arrow library, and I check your suggestion right now, and found that:
   #define ARROW_ARG_UNUSED(x) 
   then... I did not think it will works well in clang/g++, otherwise change the macro implementation to my code?




----------------------------------------------------------------
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] hurricane1026 commented on a change in pull request #7202: ARROW-8825: [C++] Fix compilation for Wunused-paraemter flag

Posted by GitBox <gi...@apache.org>.
hurricane1026 commented on a change in pull request #7202:
URL: https://github.com/apache/arrow/pull/7202#discussion_r426266743



##########
File path: cpp/src/arrow/array.h
##########
@@ -798,7 +798,12 @@ class ARROW_EXPORT FixedSizeListArray : public Array {
     i += data_->offset;
     return static_cast<int32_t>(list_size_ * i);
   }
+#ifdef _MSC_VER
   int32_t value_length(int64_t i = 0) const { return list_size_; }
+#else
+  int32_t value_length(__attribute__((unused)) int64_t i = 0) const { return list_size_; }

Review comment:
       I was not very clear about arrow library, and I check your suggestion right now, found that:
   `#define ARROW_ARG_UNUSED(x) `
   then... I did not think it will works well in clang/g++, otherwise change the macro implementation to my code?




----------------------------------------------------------------
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 #7202: ARROW-8825: [C++] Fix compilation for Wunused-paraemter flag

Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #7202:
URL: https://github.com/apache/arrow/pull/7202#discussion_r426665679



##########
File path: cpp/src/arrow/array.h
##########
@@ -798,7 +798,12 @@ class ARROW_EXPORT FixedSizeListArray : public Array {
     i += data_->offset;
     return static_cast<int32_t>(list_size_ * i);
   }
+#ifdef _MSC_VER
   int32_t value_length(int64_t i = 0) const { return list_size_; }
+#else
+  int32_t value_length(__attribute__((unused)) int64_t i = 0) const { return list_size_; }

Review comment:
       I would be surprised if this were the only place where a parameter isn't used. Also, @kou is right that this should use a macro.




----------------------------------------------------------------
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] hurricane1026 commented on a change in pull request #7202: ARROW-8825: [C++] Fix compilation for Wunused-paraemter flag

Posted by GitBox <gi...@apache.org>.
hurricane1026 commented on a change in pull request #7202:
URL: https://github.com/apache/arrow/pull/7202#discussion_r427398016



##########
File path: cpp/src/arrow/array.h
##########
@@ -798,7 +798,12 @@ class ARROW_EXPORT FixedSizeListArray : public Array {
     i += data_->offset;
     return static_cast<int32_t>(list_size_ * i);
   }
+#ifdef _MSC_VER
   int32_t value_length(int64_t i = 0) const { return list_size_; }
+#else
+  int32_t value_length(__attribute__((unused)) int64_t i = 0) const { return list_size_; }

Review comment:
       @pitrou no，very much places , so I gave it up, maybe the arrow-cpp should be implemented by more c++-like.




----------------------------------------------------------------
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