apache / incubator-graphar

An open source, standard data file format for graph data storage and retrieval.
https://graphar.apache.org/
Apache License 2.0
217 stars 46 forks source link

feat(c++): Use C++17's nested namespaces #473

Closed acezen closed 4 months ago

acezen commented 4 months ago

Describe the enhancement requested

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 graphar {
namespace util {

After:

namespace graphar::util {

Component(s)

C++

ywh555hhh commented 4 months ago

Hello, I'm interested in taking on this issue. I understand it involves implementing cleaner code. Could you please assign it to me? Thank you.

lixueclaire commented 4 months ago

Hello, I'm interested in taking on this issue. I understand it involves implementing cleaner code. Could you please assign it to me? Thank you.

Sure, the issue has been assigned to you. Thank you for your interest, and we are looking forward to your contribution.

ywh555hhh commented 4 months ago

Hello, I have a question. In the case where we have code in the graphar namespace but not in any nested namespace (like the Status class functions), should we still modify the namespace to use the C++17 nested namespace syntax? From my understanding, using the new syntax wouldn't reduce the nesting in this case. Could you please clarify this? Thank you.

here

lixueclaire commented 4 months ago

Hello, I have a question. In the case where we have code in the graphar namespace but not in any nested namespace (like the Status class functions), should we still modify the namespace to use the C++17 nested namespace syntax? From my understanding, using the new syntax wouldn't reduce the nesting in this case. Could you please clarify this? Thank you.

here

Hi, I think you could separate the code and re-org them using the new style, like:

namespace graphar::util {
...
}

namespace graphar {
...
}
ywh555hhh commented 4 months ago

Can this issue be closed?

acezen commented 4 months ago

yes, thanks for @ywh555hhh 's work, the issue has been resolved.