You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "zhjwpku (via GitHub)" <gi...@apache.org> on 2025/01/16 15:13:59 UTC
[PR] ci: add clang-tidy Checks [iceberg-cpp]
zhjwpku opened a new pull request, #32:
URL: https://github.com/apache/iceberg-cpp/pull/32
Add a .clang-tidy to explicitly specify the check rules.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: add clang-tidy Checks [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #32:
URL: https://github.com/apache/iceberg-cpp/pull/32#issuecomment-2611769691
> If we are going to use a Result/Status object, then I think we should have `[[nodiscard]]` enforcement. If we are going to use exceptions then I don't feel so strongly.
Then we can declare `class [[nodiscard]] Result` to achieve 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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: add clang-tidy Checks [iceberg-cpp]
Posted by "zhjwpku (via GitHub)" <gi...@apache.org>.
zhjwpku commented on PR #32:
URL: https://github.com/apache/iceberg-cpp/pull/32#issuecomment-2597357820
> Should we just follow what [Arrow](https://arrow.apache.org/docs/developers/cpp/development.html#code-style-linting-and-ci) does and adjust it as needed? I think it is based on the Google coding style with some modifications and some contributors are already familiar with it.
>
> I have also checked that Impala has derived from the Google style as well: https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=65868536. @gaborkaszab WDYT?
I do notice that there is a google-objc-function-naming[1] check, but it seems does report the anomaly(see [2]), i.e. `printPuffin` not reported.
[1] https://releases.llvm.org/12.0.0/tools/clang/tools/extra/docs/clang-tidy/checks/google-objc-function-naming.html
[2] https://github.com/zhjwpku/iceberg-cpp/pull/2/files
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: add clang-tidy Checks [iceberg-cpp]
Posted by "zhjwpku (via GitHub)" <gi...@apache.org>.
zhjwpku commented on PR #32:
URL: https://github.com/apache/iceberg-cpp/pull/32#issuecomment-2601912091
> I don't think we've had any major issues with our style conventions on Arrow C++.
>
> The only annoyance I have personally is the ambiguity around method naming, between `CamelCase` for most methods and `snake_case` for "cheap" accessors. I would rather a single method naming (and, coming from Python, I generally like `snake_case` the best, but that's just personal :) ).
I'm wondering if the Arrow style enforced by CI rules or just committers enforce it while reviewing PRs? It seems not easy for a bot to tell whether a method is "cheap" or not, I might be wrong though.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: add clang-tidy Checks [iceberg-cpp]
Posted by "zhjwpku (via GitHub)" <gi...@apache.org>.
zhjwpku commented on PR #32:
URL: https://github.com/apache/iceberg-cpp/pull/32#issuecomment-2612095523
Hi @Xuanwo @Fokko , I think this PR is ready to merge, please take a look when you got the time.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: add clang-tidy Checks [iceberg-cpp]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #32:
URL: https://github.com/apache/iceberg-cpp/pull/32#issuecomment-2611877332
Ah, interesting. (I am used to the std::expected backport which doesn't do this, so I always put it by hand everywhere...)
Anyways, I don't feel strongly about this. If you want to remove nodiscard then let's just proceed, I would rather have some static checks than none.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: add clang-tidy Checks [iceberg-cpp]
Posted by "zhjwpku (via GitHub)" <gi...@apache.org>.
zhjwpku commented on PR #32:
URL: https://github.com/apache/iceberg-cpp/pull/32#issuecomment-2611699882
> TBH, I'm not in favor of adding `[[nodiscard]]` every where. It is boring to type it for every function and make the function signature too verbose. How about only adding `[[nodiscard]]` to functions that will lead to fatal error if return values are discarded?
Make sense, maybe we can depend on `-Wno-unused-result` to let compiler warn if function return value is not used. As for the user interfaces, I think we should add `[[nodiscard]]`. @lidavidm thoughts?
>
> See https://stackoverflow.com/questions/77242978/what-are-the-common-practices-and-considerations-for-using-the-nodiscard-att
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: add clang-tidy Checks [iceberg-cpp]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #32:
URL: https://github.com/apache/iceberg-cpp/pull/32#issuecomment-2597296054
> Should we just follow what [Arrow](https://arrow.apache.org/docs/developers/cpp/development.html#code-style-linting-and-ci) does and adjust it as needed? I think it is based on the Google coding style with some modifications and some contributors are already familiar with it.
>
> I have also checked that Impala has derived from the Google style as well: https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=65868536. @gaborkaszab WDYT?
IIRC the one complication is that Arrow follows a bit of an old version of Google style (notably: Arrow still prefers `T*` over `T&` while Google style now prefers the latter)
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: add clang-tidy Checks [iceberg-cpp]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #32:
URL: https://github.com/apache/iceberg-cpp/pull/32#issuecomment-2612114917
> TBH, I'm not in favor of adding `[[nodiscard]]` every where. It is boring to type it for every function and make the function signature too verbose. How about only adding `[[nodiscard]]` to functions that will lead to fatal error if return values are discarded?
I also think that it does not make sense to add `[[nodiscard]]` everywhere. It should only be used when ignoring the return value would be an error, for example if the return value itself is an error 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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: add clang-tidy Checks [iceberg-cpp]
Posted by "raulcd (via GitHub)" <gi...@apache.org>.
raulcd commented on PR #32:
URL: https://github.com/apache/iceberg-cpp/pull/32#issuecomment-2612046285
> I checked that while working on this PR, both '' and file works.
Thanks for confirming, I didn't see the `file` documented and was wondering why we had it.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: add clang-tidy Checks [iceberg-cpp]
Posted by "zhjwpku (via GitHub)" <gi...@apache.org>.
zhjwpku commented on PR #32:
URL: https://github.com/apache/iceberg-cpp/pull/32#issuecomment-2596010890
This is the outcome of @lidavidm 's question whether we should use lowerCamelCase or UpperCamelCase for method names. I referenced ORC and Arrow's clang-tidy configs.
If a PR want to commit some functions not following conventions, cpp-linter-action will report suggestions, see https://github.com/zhjwpku/iceberg-cpp/pull/2 as a example.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: add clang-tidy Checks [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #32:
URL: https://github.com/apache/iceberg-cpp/pull/32#issuecomment-2611657586
TBH, I'm not in favor of adding `[[nodiscard]]` every where. It is boring to type it for every function and make the function signature too verbose. How about only adding `[[nodiscard]]` to functions that will lead to fatal error if return values are discarded?
See https://stackoverflow.com/questions/77242978/what-are-the-common-practices-and-considerations-for-using-the-nodiscard-att
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: add clang-tidy Checks [iceberg-cpp]
Posted by "zhjwpku (via GitHub)" <gi...@apache.org>.
zhjwpku commented on PR #32:
URL: https://github.com/apache/iceberg-cpp/pull/32#issuecomment-2602114984
I found a clang-tidy readability-identifier-naming for Google's naming convention [1], the gist has 7 revisions and seems quite popular(18 stars), does it make sense we adopt it?
[1] https://gist.github.com/airglow923/1fa3bda42f2b193920d7f46ee8345e04
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: add clang-tidy Checks [iceberg-cpp]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #32:
URL: https://github.com/apache/iceberg-cpp/pull/32#issuecomment-2601952277
Most of it is enforced by committers on PRs. It would probably be nice to have more automated enforcement, though.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: add clang-tidy Checks [iceberg-cpp]
Posted by "zhjwpku (via GitHub)" <gi...@apache.org>.
zhjwpku commented on PR #32:
URL: https://github.com/apache/iceberg-cpp/pull/32#issuecomment-2612040409
> I have never configured the cpp-linter action previously but don't we have to update the [tidy-checks option](https://cpp-linter.github.io/cpp-linter-action/inputs-outputs/#tidy-checks) to an empty string in order to use that config file? From the docs:
>
> > It is also possible to rely solely on a .clang-tidy config file by specifying this option as a blank string ('').
>
> My question is if this is currently being exercised and enforced on our linter CI step
I checked that while working on this PR, both '' and file works.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: add clang-tidy Checks [iceberg-cpp]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #32:
URL: https://github.com/apache/iceberg-cpp/pull/32#issuecomment-2597182577
More generally: is there a particular style guide (e.g. Google, LLVM, ...) that we want to adopt?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: add clang-tidy Checks [iceberg-cpp]
Posted by "zhjwpku (via GitHub)" <gi...@apache.org>.
zhjwpku commented on PR #32:
URL: https://github.com/apache/iceberg-cpp/pull/32#issuecomment-2597283325
> More generally: is there a particular style guide (e.g. Google, LLVM, ...) that we want to adopt?
I didn't find such one line setting, this PR sets lots of readability-identifier-naming rules, some might not be reasonable, we can remove some if someone knows it's wrong.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: add clang-tidy Checks [iceberg-cpp]
Posted by "zhjwpku (via GitHub)" <gi...@apache.org>.
zhjwpku commented on PR #32:
URL: https://github.com/apache/iceberg-cpp/pull/32#issuecomment-2597414406
> As the docs state, that's for Objective-C, so I wouldn't expect it to help
Yeah, I did not notice that ;(
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: add clang-tidy Checks [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #32:
URL: https://github.com/apache/iceberg-cpp/pull/32#issuecomment-2597282759
Should we just follow what [Arrow](https://arrow.apache.org/docs/developers/cpp/development.html#code-style-linting-and-ci) does and adjust it as needed? I think it is based on the Google coding style with some modifications and some contributors are already familiar with it.
I have also checked that Impala has derived from the Google style as well: https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=65868536. @gaborkaszab WDYT?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: add clang-tidy Checks [iceberg-cpp]
Posted by "gaborkaszab (via GitHub)" <gi...@apache.org>.
gaborkaszab commented on PR #32:
URL: https://github.com/apache/iceberg-cpp/pull/32#issuecomment-2601730266
> Should we just follow what [Arrow](https://arrow.apache.org/docs/developers/cpp/development.html#code-style-linting-and-ci) does and adjust it as needed? I think it is based on the Google coding style with some modifications and some contributors are already familiar with it.
>
> I have also checked that Impala has derived from the Google style as well: https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=65868536. @gaborkaszab WDYT?
Yeah, well I'm not sure how much of the Goggle style we keep in Impala, for me any convention is fine as long as it's well defined and enforced to everyone. I guess Arrow is well adopted enough to follow that, but frankly I have no experience with that so I'll leave this for people involved with that project.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: add clang-tidy Checks [iceberg-cpp]
Posted by "zhjwpku (via GitHub)" <gi...@apache.org>.
zhjwpku commented on PR #32:
URL: https://github.com/apache/iceberg-cpp/pull/32#issuecomment-2612048913
> > I checked that while working on this PR, both '' and file works.
>
> Thanks for confirming, I didn't see the `file` documented and was wondering why we had it.
Let's change file to '' in case future misunderstanding.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: add clang-tidy Checks [iceberg-cpp]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #32:
URL: https://github.com/apache/iceberg-cpp/pull/32#discussion_r1927188402
##########
.clang-tidy:
##########
@@ -0,0 +1,37 @@
+# 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.
+---
+Checks: |
+ -*,
+ clang-diagnostic-*,
+ clang-analyzer-*,
+ google-*,
+ modernize-*,
+ -modernize-use-nodiscard,
Review Comment:
I think we can add nodiscard. There is not much existing code anyways?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: add clang-tidy Checks [iceberg-cpp]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #32:
URL: https://github.com/apache/iceberg-cpp/pull/32#issuecomment-2610134996
Sounds good to me.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: add clang-tidy Checks [iceberg-cpp]
Posted by "zhjwpku (via GitHub)" <gi...@apache.org>.
zhjwpku commented on PR #32:
URL: https://github.com/apache/iceberg-cpp/pull/32#issuecomment-2610043044
As @lidavidm is working on the type system, we seems to prefer Arrow-style, so I'm proposing we adopt Arrow's clang-tidy.
There are two points I want to mention here:
- I did not include `-modernize-avoid-c-arrays`, we can add this later when we really want the c array style
- `-modernize-use-nodiscard` is include here, because we don't add [[nodiscard]] before the existing functions, I notice @lidavidm uses [[nodiscard]] in the type system code, so I'm wondering should we change all functions to this style?
@wgtmac @lidavidm @pitrou what do you think?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: add clang-tidy Checks [iceberg-cpp]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko merged PR #32:
URL: https://github.com/apache/iceberg-cpp/pull/32
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: add clang-tidy Checks [iceberg-cpp]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #32:
URL: https://github.com/apache/iceberg-cpp/pull/32#issuecomment-2611997925
nit: change the PR title to `ci: add clang-tidy check`
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: add clang-tidy Checks [iceberg-cpp]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #32:
URL: https://github.com/apache/iceberg-cpp/pull/32#issuecomment-2611736128
If we are going to use a Result/Status object, then I think we should have `[[nodiscard]]` enforcement. If we are going to use exceptions then I don't feel so strongly.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: add clang-tidy Checks [iceberg-cpp]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #32:
URL: https://github.com/apache/iceberg-cpp/pull/32#issuecomment-2601757647
I don't think we've had any major issues with our style conventions on Arrow C++.
The only annoyance I have personally is the ambiguity around method naming, between `CamelCase` for most methods and `snake_case` for "cheap" accessors. I would rather a single method naming (and, coming from Python, I generally like `snake_case` the best, but that's just personal :) ).
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: add clang-tidy Checks [iceberg-cpp]
Posted by "zhjwpku (via GitHub)" <gi...@apache.org>.
zhjwpku commented on code in PR #32:
URL: https://github.com/apache/iceberg-cpp/pull/32#discussion_r1927244347
##########
.clang-tidy:
##########
@@ -0,0 +1,37 @@
+# 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.
+---
+Checks: |
+ -*,
+ clang-diagnostic-*,
+ clang-analyzer-*,
+ google-*,
+ modernize-*,
+ -modernize-use-nodiscard,
Review Comment:
agreed, done that.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] ci: add clang-tidy Checks [iceberg-cpp]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #32:
URL: https://github.com/apache/iceberg-cpp/pull/32#issuecomment-2597391057
As the docs state, that's for Objective-C, so I wouldn't expect it 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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org