fix(c++): using C++17's nested namespaces #489

ywh555hhh · 2024-05-22T05:21:25Z

Initial modifications to feat(c++): Use C++17's nested namespaces #473 completed, involving content of anonymous namespace and content in result.hpp not changed yet

Reason for this PR

to slove the issue #473

What changes are included in this PR?

Most cpp folders need to use cpp17 nested loops

Are these changes tested?

No local tests

Are there any user-facing changes?

no

Initial modifications to feat(c++): Use C++17's nested namespaces apache#473 completed, involving content of anonymous namespace and content in result.hpp not changed yet

fix: Corrected minor errors in uri_parser.h

ywh555hhh · 2024-05-22T06:40:52Z

I seem to be ignoring some of the call logic for nested namespaces and I'll fix it myself

acezen · 2024-05-22T06:46:53Z

Hi @ywh555hhh, thanks for the work, the CI indicate that the code format check is failed. Please follow the

incubator-graphar/.github/workflows/ci.yml

Lines 82 to 83 in 87d5eeb

    
                   sudo curl -L https://github.com/muttleyxd/clang-tools-static-binaries/releases/download/master-22538c65/clang-format-8_linux-amd64 --output /usr/bin/clang-format 
        
                   sudo chmod +x /usr/bin/clang-format

to install clang-format 8.0 and fix the format with

make graphar-clformat

acezen · 2024-05-22T07:10:33Z

cpp/thirdparty/simple-uri-parser/uri_parser.h

@@ -89,7 +89,7 @@ struct Uri {
        {}
 };

-}


we better to keep the thirdparty code as their origin, not to modify them.

ywh555hhh · 2024-05-22T07:33:45Z

I encountered an error when trying to open a project in a container using vscode's remote container plug-in. How can I solve it?

Error response from daemon: pull access denied for registry.cn-hongkong.aliyuncs.com/graphscope/graphar-dev, repository does not exist or may require 'docker login': denied: requested access to the resource is denied

acezen · 2024-05-22T07:58:30Z

I encountered an error when trying to open a project in a container using vscode's remote container plug-in. How can I solve it?

Error response from daemon: pull access denied for registry.cn-hongkong.aliyuncs.com/graphscope/graphar-dev, repository does not exist or may require 'docker login': denied: requested access to the resource is denied

It seems that the image has been expired, I have re-pushed it, can you try again?

ywh555hhh · 2024-05-22T08:22:58Z

info2.txt
I've tried it with both macos and linux and it still doesn't seem to work

acezen · 2024-05-22T08:54:33Z

info2.txt I've tried it with both macos and linux and it still doesn't seem to work

It seems that the image is a private repository, thanks for report the error.
I have created an issue #491 to fix this.

acezen · 2024-05-22T08:58:36Z

info2.txt I've tried it with both macos and linux and it still doesn't seem to work

It seems that the image is a private repository, thanks for report the error. I have created an issue #491 to fix this.

Hi, @ywh555hhh, the image has been updated, could you rebase the main and try again? sorry for the obstruction.

ywh555hhh · 2024-05-22T09:02:26Z

I'm happy to contribute to the community, but I also want to know how to solve the formatting problem. If I can't use docker right now, it means I need to fix the problem on macOS

acezen · 2024-05-22T09:08:33Z

'm happy to contribute to the community, but I also want to know how to solve the formatting problem. If I can't use docker right now, it means I need to fix the problem on macOS

the clang-format-8 is also available for macos:

curl -L https://github.com/muttleyxd/clang-tools-static-binaries/releases/download/master-22538c65/clang-format-8_macosx-amd64 --output clang-format

or you can edit the code directly and fix the format base on the information provided by CI:
https://github.com/apache/incubator-graphar/actions/runs/9188486680/job/25270239169?pr=489

cpp/include/graphar/util/filesystem.h

cpp/include/graphar/fwd.h

cpp/include/graphar/util/result.h

acezen · 2024-05-22T11:34:15Z

cpp/src/edges_builder.cc

@@ -349,5 +348,4 @@ Result<std::shared_ptr<arrow::Table>> EdgesBuilder::getOffsetTable(
  return arrow::Table::Make(schema, arrays);
 }

-}  // namespace builder
-}  // namespace graphar
+}  // namespace graphar::builder


cpp/src/filesystem.cc

cpp/src/reader_util.cc

cpp/src/util.cc

cpp/src/vertices_builder.cc

acezen · 2024-05-22T11:52:39Z

It's better not to delete the empty line in the end of the source file. FYI
https://stackoverflow.com/questions/2287967/why-is-it-recommended-to-have-empty-line-in-the-end-of-a-source-file

acezen

Seems that you have changed the permission of cpp/misc/cpplint.py, is there a reason to change this?

ywh555hhh · 2024-05-22T11:59:07Z

Seems that you have changed the permission of cpp/misc/cpplint.py, is there a reason to change this?

I had no intention of doing that. Could this have been some kind of operational error that I wasn't aware of. Sorry

acezen · 2024-05-22T12:03:05Z

Seems that you have changed the permission of cpp/misc/cpplint.py, is there a reason to change this?

I had no intention of doing that. Could this have been some kind of operational error that I wasn't aware of. Sorry

It would make the CI failed. see
https://github.com/apache/incubator-graphar/actions/runs/9190636917/job/25275644225?pr=489

You can revert it to the original commit with git
https://stackoverflow.com/questions/215718/how-can-i-reset-or-revert-a-file-to-a-specific-revision

ywh555hhh · 2024-05-22T12:14:58Z

Seems that you have changed the permission of cpp/misc/cpplint.py, is there a reason to change this?

I had no intention of doing that. Could this have been some kind of operational error that I wasn't aware of. Sorry

It would make the CI failed. see https://github.com/apache/incubator-graphar/actions/runs/9190636917/job/25275644225?pr=489

You can revert it to the original commit with git https://stackoverflow.com/questions/215718/how-can-i-reset-or-revert-a-file-to-a-specific-revision

Thanks for the Pointers. I really appreciate you taking the time to mentor me

acezen · 2024-05-22T12:31:13Z

Seems that you have changed the permission of cpp/misc/cpplint.py, is there a reason to change this?

I had no intention of doing that. Could this have been some kind of operational error that I wasn't aware of. Sorry

It would make the CI failed. see https://github.com/apache/incubator-graphar/actions/runs/9190636917/job/25275644225?pr=489
You can revert it to the original commit with git https://stackoverflow.com/questions/215718/how-can-i-reset-or-revert-a-file-to-a-specific-revision

Thanks for the Pointers. I really appreciate you taking the time to mentor me

You are welcome. We are pleased to have your contribution.

ywh555hhh · 2024-05-22T13:24:25Z

I'll give clang-format-8 a try

cpp/include/graphar/util/filesystem.h

cpp/include/graphar/util/reader_util.h

…d-namespaces

ywh555hhh · 2024-05-23T01:53:40Z

The version of clang-format I used was wrong and I will try to install the required version myself

This reverts commit c215171.

ywh555hhh · 2024-05-23T02:21:33Z

yiweihan@yiweihandeMacBook-Air cpp %  clang-format --version
clang-format version 8.0.1 (tags/RELEASE_801/final)
yiweihan@yiweihandeMacBook-Air cpp % make clformat          
make: *** No rule to make target `clformat'.  Stop.
yiweihan@yiweihandeMacBook-Air cpp %

I have successfully installed clang-format but I can't run the clformat command. What is the problem? And the cpp folder that I noticed contains the.clang-format file, which says the format standard

acezen · 2024-05-23T02:28:11Z

yiweihan@yiweihandeMacBook-Air cpp %  clang-format --version
clang-format version 8.0.1 (tags/RELEASE_801/final)
yiweihan@yiweihandeMacBook-Air cpp % make clformat          
make: *** No rule to make target `clformat'.  Stop.
yiweihan@yiweihandeMacBook-Air cpp % 
I have successfully installed clang-format but I can't run the clformat command. What is the problem? And the cpp folder that I noticed contains the.clang-format file, which says the format standard

the command

make graphar-clformat

is run in the build directory if you create a build dir

ywh555hhh · 2024-05-23T02:38:30Z

yiweihan@yiweihandeMacBook-Air cpp %  clang-format --version
clang-format version 8.0.1 (tags/RELEASE_801/final)
yiweihan@yiweihandeMacBook-Air cpp % make clformat          
make: *** No rule to make target `clformat'.  Stop.
yiweihan@yiweihandeMacBook-Air cpp % 
I have successfully installed clang-format but I can't run the clformat command. What is the problem? And the cpp folder that I noticed contains the.clang-format file, which says the format standard
the command
make graphar-clformat
is run in the build directory if you create a build dir

yiweihan@yiweihandeMacBook-Air cpp %  clang-format --version
clang-format version 8.0.1 (tags/RELEASE_801/final)
yiweihan@yiweihandeMacBook-Air cpp % make clformat          
make: *** No rule to make target `clformat'.  Stop.
yiweihan@yiweihandeMacBook-Air cpp % make graphar-clformat
make: *** No rule to make target `graphar-clformat'.  Stop.
yiweihan@yiweihandeMacBook-Air cpp % cd ..
yiweihan@yiweihandeMacBook-Air incubator-graphar % mkdir build
yiweihan@yiweihandeMacBook-Air incubator-graphar % cd build
yiweihan@yiweihandeMacBook-Air build % make graphar-clformat
make: *** No rule to make target `graphar-clformat'.  Stop.
yiweihan@yiweihandeMacBook-Air build %

Did I misunderstand something? I don't even seem to be looking for the Makefile

acezen

Overall LGTM, and there have some nits to be fixed

cpp/include/graphar/util/reader_util.h

acezen · 2024-05-23T02:33:32Z

cpp/include/graphar/util/status.h

-namespace graphar {
-
-namespace util {
+namespace graphar::util {


add an empty line before namespace for code clarity

acezen · 2024-05-23T02:35:02Z

cpp/include/graphar/writer/edges_builder.h

-
-namespace graphar {
-namespace builder {
+namespace graphar::builder {


add an empty line before namespace for code clarity

acezen · 2024-05-23T02:35:47Z

cpp/src/filesystem.cc

-namespace graphar {
-namespace ds = arrow::dataset;
-namespace detail {
+namespace graphar::detail {


here need empty line between header include and implementation

acezen · 2024-05-23T02:46:16Z

cpp/src/filesystem.cc

@@ -79,7 +76,9 @@ static Status CastToLargeOffsetArray(
  GAR_RETURN_ON_ARROW_ERROR_AND_ASSIGN(out, arrow::ChunkedArray::Make(chunks));
  return Status::OK();
 }
-}  // namespace detail
+}  // namespace graphar::detail


Add empty line here for code clarify

acezen · 2024-05-23T02:48:08Z

yiweihan@yiweihandeMacBook-Air cpp %  clang-format --version
clang-format version 8.0.1 (tags/RELEASE_801/final)
yiweihan@yiweihandeMacBook-Air cpp % make clformat          
make: *** No rule to make target `clformat'.  Stop.
yiweihan@yiweihandeMacBook-Air cpp % 
I have successfully installed clang-format but I can't run the clformat command. What is the problem? And the cpp folder that I noticed contains the.clang-format file, which says the format standard
the command
make graphar-clformat
is run in the build directory if you create a build dir

yiweihan@yiweihandeMacBook-Air cpp %  clang-format --version
clang-format version 8.0.1 (tags/RELEASE_801/final)
yiweihan@yiweihandeMacBook-Air cpp % make clformat          
make: *** No rule to make target `clformat'.  Stop.
yiweihan@yiweihandeMacBook-Air cpp % make graphar-clformat
make: *** No rule to make target `graphar-clformat'.  Stop.
yiweihan@yiweihandeMacBook-Air cpp % cd ..
yiweihan@yiweihandeMacBook-Air incubator-graphar % mkdir build
yiweihan@yiweihandeMacBook-Air incubator-graphar % cd build
yiweihan@yiweihandeMacBook-Air build % make graphar-clformat
make: *** No rule to make target `graphar-clformat'.  Stop.
yiweihan@yiweihandeMacBook-Air build %

Did I misunderstand something? I don't even seem to be looking for the Makefile

you need to run cmake .. before make. It's the same with building process

ywh555hhh · 2024-05-23T03:07:32Z

yiweihan@yiweihandeMacBook-Air cpp %  clang-format --version
clang-format version 8.0.1 (tags/RELEASE_801/final)
yiweihan@yiweihandeMacBook-Air cpp % make clformat          
make: *** No rule to make target `clformat'.  Stop.
yiweihan@yiweihandeMacBook-Air cpp % 
I have successfully installed clang-format but I can't run the clformat command. What is the problem? And the cpp folder that I noticed contains the.clang-format file, which says the format standard
the command
make graphar-clformat
is run in the build directory if you create a build dir

yiweihan@yiweihandeMacBook-Air cpp %  clang-format --version
clang-format version 8.0.1 (tags/RELEASE_801/final)
yiweihan@yiweihandeMacBook-Air cpp % make clformat          
make: *** No rule to make target `clformat'.  Stop.
yiweihan@yiweihandeMacBook-Air cpp % make graphar-clformat
make: *** No rule to make target `graphar-clformat'.  Stop.
yiweihan@yiweihandeMacBook-Air cpp % cd ..
yiweihan@yiweihandeMacBook-Air incubator-graphar % mkdir build
yiweihan@yiweihandeMacBook-Air incubator-graphar % cd build
yiweihan@yiweihandeMacBook-Air build % make graphar-clformat
make: *** No rule to make target `graphar-clformat'.  Stop.
yiweihan@yiweihandeMacBook-Air build %

Did I misunderstand something? I don't even seem to be looking for the Makefile

you need to run cmake .. before make. It's the same with building process

tks I've tried it. Can this be merged now?

ywh555hhh · 2024-05-23T03:16:32Z

Overall LGTM, and there have some nits to be fixed

ok👌🏻

Initial modifications to feat(c++): Use C++17's nested namespaces apache#473 completed, involving content of anonymous namespace and content in result.hpp not changed yet Reason for this PR to slove the issue apache#473 What changes are included in this PR? Most cpp folders need to use cpp17 nested loops Are these changes tested? No local tests Are there any user-facing changes? no

ywh555hhh added 2 commits May 22, 2024 12:55

Partial modifications to the nested namespace

e040c6d

Initial modifications to feat(c++): Use C++17's nested namespaces apache#473 completed, involving content of anonymous namespace and content in result.hpp not changed yet

fix: Corrected minor errors in uri_parser.h

fa9da75

fix: Corrected minor errors in uri_parser.h

ywh555hhh changed the title ~~fix: Resolved the issue of using C++17's nested namespaces #473~~ fix: #473 Resolved the issue of using C++17's nested namespaces May 22, 2024

Update status.h

9ac3a90

acezen reviewed May 22, 2024

View reviewed changes

fix：nested namespace

d14d264

Merge branch 'apache:main' into 473-use-cpp17-nested-namespaces

73cbd69

ywh555hhh added 2 commits May 22, 2024 18:45

fit cpp format

41494c0

format fix 2

c3786b5

acezen reviewed May 22, 2024

View reviewed changes

Update filesystem.h

77fb366

acezen reviewed May 22, 2024

View reviewed changes

fix format

44fa161

fix cpplint.py

ede96c0

fix

3481808

acezen reviewed May 23, 2024

View reviewed changes

cpp/include/graphar/util/filesystem.h Outdated Show resolved Hide resolved

acezen reviewed May 23, 2024

View reviewed changes

cpp/include/graphar/util/reader_util.h Outdated Show resolved Hide resolved

ywh555hhh added 3 commits May 23, 2024 09:31

Merge remote-tracking branch 'upstream/main' into 473-use-cpp17-neste…

8f2cf66

…d-namespaces

fix format

e369280

fix with clang-format

c215171

ywh555hhh added 2 commits May 23, 2024 09:57

Revert "fix with clang-format"

1d14965

This reverts commit c215171.

fix format

6723507

acezen changed the title ~~fix: #473 Resolved the issue of using C++17's nested namespaces~~ fix(c++): using C++17's nested namespaces May 23, 2024

acezen reviewed May 23, 2024

View reviewed changes

ywh555hhh added 2 commits May 23, 2024 11:22

fix nits

5165aa0

fix format

53023e8

acezen approved these changes May 23, 2024

View reviewed changes

acezen merged commit d8abc0e into apache:main May 23, 2024
3 checks passed

acezen mentioned this pull request Jun 12, 2024

feat(c++,spark): support json payload file format #518

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(c++): using C++17's nested namespaces #489

fix(c++): using C++17's nested namespaces #489

ywh555hhh commented May 22, 2024

ywh555hhh commented May 22, 2024

acezen commented May 22, 2024

acezen May 22, 2024

ywh555hhh May 22, 2024

ywh555hhh commented May 22, 2024

acezen commented May 22, 2024

ywh555hhh commented May 22, 2024

acezen commented May 22, 2024

acezen commented May 22, 2024 •

edited

Loading

ywh555hhh commented May 22, 2024

acezen commented May 22, 2024 •

edited

Loading

acezen May 22, 2024

acezen commented May 22, 2024

acezen left a comment •

edited

Loading

ywh555hhh commented May 22, 2024 •

edited

Loading

acezen commented May 22, 2024 •

edited

Loading

ywh555hhh commented May 22, 2024

acezen commented May 22, 2024

ywh555hhh commented May 22, 2024

ywh555hhh commented May 23, 2024

ywh555hhh commented May 23, 2024

acezen commented May 23, 2024

ywh555hhh commented May 23, 2024

acezen left a comment

acezen May 23, 2024

acezen May 23, 2024

acezen May 23, 2024

acezen May 23, 2024

acezen commented May 23, 2024

ywh555hhh commented May 23, 2024

ywh555hhh commented May 23, 2024

@@ @@ -89,7 +89,7 @@ struct Uri { @@
                       {}
               };
-              }

fix(c++): using C++17's nested namespaces #489

fix(c++): using C++17's nested namespaces #489

Conversation

ywh555hhh commented May 22, 2024

Reason for this PR

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

ywh555hhh commented May 22, 2024

acezen commented May 22, 2024

acezen May 22, 2024

Choose a reason for hiding this comment

ywh555hhh May 22, 2024

Choose a reason for hiding this comment

ywh555hhh commented May 22, 2024

acezen commented May 22, 2024

ywh555hhh commented May 22, 2024

acezen commented May 22, 2024

acezen commented May 22, 2024 • edited Loading

ywh555hhh commented May 22, 2024

acezen commented May 22, 2024 • edited Loading

acezen May 22, 2024

Choose a reason for hiding this comment

acezen commented May 22, 2024

acezen left a comment • edited Loading

Choose a reason for hiding this comment

ywh555hhh commented May 22, 2024 • edited Loading

acezen commented May 22, 2024 • edited Loading

ywh555hhh commented May 22, 2024

acezen commented May 22, 2024

ywh555hhh commented May 22, 2024

ywh555hhh commented May 23, 2024

ywh555hhh commented May 23, 2024

acezen commented May 23, 2024

ywh555hhh commented May 23, 2024

acezen left a comment

Choose a reason for hiding this comment

acezen May 23, 2024

Choose a reason for hiding this comment

acezen May 23, 2024

Choose a reason for hiding this comment

acezen May 23, 2024

Choose a reason for hiding this comment

acezen May 23, 2024

Choose a reason for hiding this comment

acezen commented May 23, 2024

ywh555hhh commented May 23, 2024

ywh555hhh commented May 23, 2024

acezen commented May 22, 2024 •

edited

Loading

acezen commented May 22, 2024 •

edited

Loading

acezen left a comment •

edited

Loading

ywh555hhh commented May 22, 2024 •

edited

Loading

acezen commented May 22, 2024 •

edited

Loading