Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed, CXXGraph::Node::getData() should not be const #438 #462

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

yapa-ymtl
Copy link

Hi @ZigRazor
This PR related to "CXXGraph::Node::getData() should not be const" issue with #438.

@ZigRazor ZigRazor linked an issue Aug 6, 2024 that may be closed by this pull request
@ZigRazor
Copy link
Owner

ZigRazor commented Aug 6, 2024

the code does not compile... please check it

@yapa-ymtl
Copy link
Author

Thanks , I am checking that

Copy link

codecov bot commented Aug 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.87%. Comparing base (564a8c0) to head (4bfa986).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #462   +/-   ##
=======================================
  Coverage   97.87%   97.87%           
=======================================
  Files          87       87           
  Lines       10079    10081    +2     
  Branches      670      670           
=======================================
+ Hits         9865     9867    +2     
  Misses        214      214           
Flag Coverage Δ
unittests 97.87% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ZigRazor
Copy link
Owner

ZigRazor commented Aug 7, 2024

The workflow of build and test does not work in any platform( only windows fails in general ), but for ubuntu and macos the workflow should compile and test the library.
Please check the problem and fix it.
Thank you

@@ -48,6 +48,7 @@ class Node {
const CXXGraph::id_t &getId() const;
const std::string &getUserId() const;
const T &getData() const;
T &getData() ;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the extra space before the semicolon. And the & should be next to T. Additionally, the function can be marked const (however the type returned should not be const T&).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i.e. T& getData() const;

Copy link
Author

@yapa-ymtl yapa-ymtl Aug 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is not possible, We cannot overload a function based on return type.

@@ -65,6 +65,11 @@ const T &Node<T>::getData() const {
return data;
}

template <typename T>
T &Node<T>::getData() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here - the & should be next to the T.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will update

@nolankramer
Copy link
Collaborator

@ZigRazor It seems the tests are failing for reasons unrelated to the code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CXXGraph::Node::getData() should not be const
3 participants