Fixed, CXXGraph::Node::getData() should not be const #438#462
Fixed, CXXGraph::Node::getData() should not be const #438#462ZigRazor merged 6 commits intoZigRazor:masterfrom
Conversation
|
the code does not compile... please check it |
|
Thanks , I am checking that |
Codecov ReportAll modified and coverable lines are covered by tests ✅
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
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. |
include/CXXGraph/Node/Node_decl.h
Outdated
| const CXXGraph::id_t &getId() const; | ||
| const std::string &getUserId() const; | ||
| const T &getData() const; | ||
| T &getData() ; |
There was a problem hiding this comment.
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&).
There was a problem hiding this comment.
i.e. T& getData() const;
There was a problem hiding this comment.
I think this is not possible, We cannot overload a function based on return type.
There was a problem hiding this comment.
You could delete the overload on a lvalue reference or return a (std::)moved object to avoid bug prone code? This is a little out of scope for the original issue.
Would look like:
const T& getData() && const = delete;
T& getData() && = delete;And you'd have to add & to the other methods
| } | ||
|
|
||
| template <typename T> | ||
| T &Node<T>::getData() { |
There was a problem hiding this comment.
Same here - the & should be next to the T.
|
@ZigRazor It seems the tests are failing for reasons unrelated to the code. |
Fixed lint issue
|
@nolankramer they fail on Windows currently because CMake doesn't like the backwards slashes ( @yapa-ymtl If you go to your repo and click actions then the big green button (enable?) you will be able to run the tests and compile it on a CI pipeline before launching a PR so you'll run into less comments like:
Also run clang format to fix the formatting issues, it will give you feedback much more quickly! |
fixed lint issue
4 participants
Hi @ZigRazor
This PR related to "CXXGraph::Node::getData() should not be const" issue with #438.