search

apache / arrow

Apache Arrow is the universal columnar format and multi-language toolbox for fast data interchange and in-memory analytics
https://arrow.apache.org/
Apache License 2.0
14.52k stars 3.54k forks source link

[C++] Use C++17's nested namespaces #36620

Open westonpace opened 1 year ago

westonpace commented 1 year ago

Describe the enhancement requested

I'm seeing some PRs start to prefer the nested namespace notation. I know clang-tidy prefers it. It is more compact and now that we are on C++ 17 I don't see any reason we shouldn't move everything over.

Before:

namespace arrow {
namespace util {
namespace internal {

After:

namespace arrow::util::internal {

Sub issues:

Component(s)

C++

mapleFU commented 1 year ago

I can finish this in parquet, should I create an extra issue for that?

westonpace commented 1 year ago

I can finish this in parquet, should I create an extra issue for that?

Sure, that would probably be best. Then we can leave this one open for everything else. Thanks!

tanishasaheb15 commented 1 year ago

is this issue still open ?

OmkarTipugade commented 9 months ago

Hi, everyone I am a beginner in open source, and I am interested in working on this issue please someone help and guides me on, how to start I know C, C++, and Java programming languages

IndifferentArea commented 3 months ago

take

IndifferentArea commented 3 months ago

This issue can be easily fixed by running clang-tidy --checks='-*,modernize-concat-nested-namespaces' --fix on all source files, but there are still 2 problems I'm not sure about:

  1. as for the comment following the right end of brackets, like

    namespace A {
namespace B {
...
} // namespace B
} // namespace A

    clang-tidy will fix it with comments not changed.

    namespace A::B {

} // namespace A

    do I need to fix it too?

  2. Do I need to fix code generators? eg https://github.com/apache/arrow/blob/299ad7086928c555f3e861c20327276d1c4f2557/cpp/src/arrow/util/bpacking_simd_codegen.py#L170-L172

Could you give me some advice? thx @westonpace

kou commented 3 months ago
  1. Could you try pre-commit run --all?
  2. Yes.
IndifferentArea commented 3 months ago

I've submitted a patch in #43420. While there are still some unresolved cases there, I believe the majority have been addressed.

Ideally, running clang-tidy should resolve this issue effortlessly. But in practice, some code in gandiva and flight/transport is hard for me to build successfully.

In addition, clang-tidy encounters a bug when dealing with nested namespaces interspersed with macros. see details in

so must be careful!