You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by echobravopapa <gi...@git.apache.org> on 2017/07/06 21:12:37 UTC
[GitHub] geode-native pull request #107: GEODE-3019: Refactor Struct class
GitHub user echobravopapa opened a pull request:
https://github.com/apache/geode-native/pull/107
GEODE-3019: Refactor Struct class
- Using std::string now
- this also addresses GEODE-3020, the map now used standard types
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/echobravopapa/geode-native feature/GEODE-3019
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/geode-native/pull/107.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #107
----
commit a502ce378d26ce374b51c4198f93bbf7d70116a6
Author: Ernest Burghardt <eb...@pivotal.io>
Date: 2017-06-30T21:42:01Z
GEODE-3019: Refactor Struct class
- Using std::string now
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode-native pull request #107: GEODE-3019: Refactor Struct class
Posted by dgkimura <gi...@git.apache.org>.
Github user dgkimura commented on a diff in the pull request:
https://github.com/apache/geode-native/pull/107#discussion_r128660879
--- Diff: src/cppcache/src/StructSetImpl.cpp ---
@@ -66,25 +66,22 @@ SelectResultsIterator StructSetImpl::getIterator() {
return SelectResultsIterator(m_structVector, shared_from_this());
}
-int32_t StructSetImpl::getFieldIndex(const char* fieldname) {
- std::map<std::string, int32_t>::iterator iter =
+const int32_t StructSetImpl::getFieldIndex(const std::string& fieldname) {
+ const auto& iter =
m_fieldNameIndexMap.find(fieldname);
if (iter != m_fieldNameIndexMap.end()) {
return iter->second;
} else {
- // std::string tmp = "fieldname ";
- // tmp += fieldname + " not found";
- throw IllegalArgumentException("fieldname not found");
+ throw std::invalid_argument("fieldname not found");
}
}
-const char* StructSetImpl::getFieldName(int32_t index) {
- for (std::map<std::string, int32_t>::iterator iter =
- m_fieldNameIndexMap.begin();
- iter != m_fieldNameIndexMap.end(); ++iter) {
- if (iter->second == index) return iter->first.c_str();
+const std::string& StructSetImpl::getFieldName(int32_t index) {
+ for (const auto& iter : m_fieldNameIndexMap) {
+ if (iter.second == index) return (iter.first);
}
- return nullptr;
+
+ throw std::out_of_range("Struct: fieldName not found.");
--- End diff --
Would it add any value having the index in the error message?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode-native issue #107: GEODE-3019: Refactor Struct class
Posted by echobravopapa <gi...@git.apache.org>.
Github user echobravopapa commented on the issue:
https://github.com/apache/geode-native/pull/107
@dgkimura @pivotal-jbarrett added unit tests and throwing an exception for out of range now
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode-native pull request #107: GEODE-3019: Refactor Struct class
Posted by dgkimura <gi...@git.apache.org>.
Github user dgkimura commented on a diff in the pull request:
https://github.com/apache/geode-native/pull/107#discussion_r126028200
--- Diff: src/cppcache/integration-test/testThinClientRemoteQuerySS.cpp ---
@@ -56,59 +57,63 @@ const wchar_t* checkNullString(const wchar_t* str) {
return ((str == nullptr) ? L"(null)" : str);
}
+std::string checkNullString(const std::string* str) {
+ return ((str == nullptr) ? "(null)" : *str);
+}
+
void _printFields(CacheablePtr field, Struct* ssptr, int32_t& fields) {
if (auto portfolio = std::dynamic_pointer_cast<Portfolio>(field)) {
printf(" pulled %s :- ID %d, pkid %s\n",
- checkNullString(ssptr->getFieldName(fields)), portfolio->getID(),
+ checkNullString(ssptr->getFieldName(fields)).c_str(), portfolio->getID(),
--- End diff --
If we can make `getFieldName` return `std::string` then I'd think we can just `+` concatenate the strings without having to do `checkNullString` and `c_str()`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode-native issue #107: GEODE-3019: Refactor Struct class
Posted by echobravopapa <gi...@git.apache.org>.
Github user echobravopapa commented on the issue:
https://github.com/apache/geode-native/pull/107
@dgkimura @pivotal-jbarrett I squashed the latest so you won't get notified, which is a bummer, but please have a look when you have a minute.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode-native pull request #107: GEODE-3019: Refactor Struct class
Posted by echobravopapa <gi...@git.apache.org>.
Github user echobravopapa commented on a diff in the pull request:
https://github.com/apache/geode-native/pull/107#discussion_r127315288
--- Diff: src/cppcache/src/StructSetImpl.cpp ---
@@ -78,13 +78,12 @@ int32_t StructSetImpl::getFieldIndex(const char* fieldname) {
}
}
-const char* StructSetImpl::getFieldName(int32_t index) {
- for (std::map<std::string, int32_t>::iterator iter =
- m_fieldNameIndexMap.begin();
- iter != m_fieldNameIndexMap.end(); ++iter) {
- if (iter->second == index) return iter->first.c_str();
+const std::string& StructSetImpl::getFieldName(int32_t index) {
+ for (const auto& iter : m_fieldNameIndexMap) {
+ if (iter.second == index) return (iter.first);
}
- return nullptr;
+
+ throw OutOfRangeException("Struct: fieldName not found.");
--- End diff --
I'm changing both of them, it is the direction the code is headed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode-native pull request #107: GEODE-3019: Refactor Struct class
Posted by echobravopapa <gi...@git.apache.org>.
Github user echobravopapa commented on a diff in the pull request:
https://github.com/apache/geode-native/pull/107#discussion_r127285563
--- Diff: src/cppcache/src/StructSetImpl.cpp ---
@@ -78,13 +78,12 @@ int32_t StructSetImpl::getFieldIndex(const char* fieldname) {
}
}
-const char* StructSetImpl::getFieldName(int32_t index) {
- for (std::map<std::string, int32_t>::iterator iter =
- m_fieldNameIndexMap.begin();
- iter != m_fieldNameIndexMap.end(); ++iter) {
- if (iter->second == index) return iter->first.c_str();
+const std::string& StructSetImpl::getFieldName(int32_t index) {
+ for (const auto& iter : m_fieldNameIndexMap) {
+ if (iter.second == index) return (iter.first);
}
- return nullptr;
+
+ throw OutOfRangeException("Struct: fieldName not found.");
--- End diff --
we can do that, I was just using our existing exception
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode-native pull request #107: GEODE-3019: Refactor Struct class
Posted by pivotal-jbarrett <gi...@git.apache.org>.
Github user pivotal-jbarrett commented on a diff in the pull request:
https://github.com/apache/geode-native/pull/107#discussion_r126792435
--- Diff: src/cppcache/src/Struct.cpp ---
@@ -96,16 +99,16 @@ Serializable* Struct::fromData(DataInput& input) {
return this;
}
-const char* Struct::getFieldName(const int32_t index) const {
+const std::string& Struct::getFieldName(const int32_t index) const {
if (m_parent != nullptr) {
return m_parent->getFieldName(index);
} else {
for (const auto& iter : m_fieldNames) {
- if ((iter.second)->value() == index) return iter.first->asChar();
+ if (iter.second == index) return (iter.first);
}
}
- return nullptr;
+ return std::string("");
--- End diff --
What about throwing [`std::out_of_range`](http://en.cppreference.com/w/cpp/error/out_of_range). I don't like the idea of checking for empty string.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode-native pull request #107: GEODE-3019: Refactor Struct class
Posted by pivotal-jbarrett <gi...@git.apache.org>.
Github user pivotal-jbarrett commented on a diff in the pull request:
https://github.com/apache/geode-native/pull/107#discussion_r127278631
--- Diff: src/cppcache/src/StructSetImpl.cpp ---
@@ -78,13 +78,12 @@ int32_t StructSetImpl::getFieldIndex(const char* fieldname) {
}
}
-const char* StructSetImpl::getFieldName(int32_t index) {
- for (std::map<std::string, int32_t>::iterator iter =
- m_fieldNameIndexMap.begin();
- iter != m_fieldNameIndexMap.end(); ++iter) {
- if (iter->second == index) return iter->first.c_str();
+const std::string& StructSetImpl::getFieldName(int32_t index) {
+ for (const auto& iter : m_fieldNameIndexMap) {
+ if (iter.second == index) return (iter.first);
}
- return nullptr;
+
+ throw OutOfRangeException("Struct: fieldName not found.");
--- End diff --
why not `std::out_of_range`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode-native issue #107: GEODE-3019: Refactor Struct class
Posted by echobravopapa <gi...@git.apache.org>.
Github user echobravopapa commented on the issue:
https://github.com/apache/geode-native/pull/107
@dgkimura @pivotal-jbarrett I think this is all ready for final inspection
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode-native pull request #107: GEODE-3019: Refactor Struct class
Posted by pivotal-jbarrett <gi...@git.apache.org>.
Github user pivotal-jbarrett commented on a diff in the pull request:
https://github.com/apache/geode-native/pull/107#discussion_r126115701
--- Diff: src/cppcache/include/geode/Struct.hpp ---
@@ -138,7 +138,7 @@ class CPPCACHE_EXPORT Struct : public Serializable {
* Returns the name of the field corresponding to the index number in the
* Struct
*/
- virtual const char* getFieldName(const int32_t index) const;
+ virtual const std::string* getFieldName(const int32_t index) const;
--- End diff --
I am too not a huge fan of returning `std::string*` but the current behavior is to return `nullptr` if the field does not exist at that index.
@dgkimura would you be happier if this method threw [`out_of_range`](http://en.cppreference.com/w/cpp/error/out_of_range)? I think I might like that better than null.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode-native pull request #107: GEODE-3019: Refactor Struct class
Posted by pivotal-jbarrett <gi...@git.apache.org>.
Github user pivotal-jbarrett commented on a diff in the pull request:
https://github.com/apache/geode-native/pull/107#discussion_r126792525
--- Diff: src/cppcache/src/StructSetImpl.cpp ---
@@ -78,13 +78,11 @@ int32_t StructSetImpl::getFieldIndex(const char* fieldname) {
}
}
-const char* StructSetImpl::getFieldName(int32_t index) {
- for (std::map<std::string, int32_t>::iterator iter =
- m_fieldNameIndexMap.begin();
- iter != m_fieldNameIndexMap.end(); ++iter) {
- if (iter->second == index) return iter->first.c_str();
+const std::string& StructSetImpl::getFieldName(int32_t index) {
+ for (const auto& iter : m_fieldNameIndexMap) {
+ if (iter.second == index) return (iter.first);
}
- return nullptr;
+ return std::string("");
--- End diff --
`std::out_of_range`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode-native pull request #107: GEODE-3019: Refactor Struct class
Posted by pivotal-jbarrett <gi...@git.apache.org>.
Github user pivotal-jbarrett commented on a diff in the pull request:
https://github.com/apache/geode-native/pull/107#discussion_r127278443
--- Diff: src/cppcache/src/StructSetImpl.cpp ---
@@ -66,8 +66,8 @@ SelectResultsIterator StructSetImpl::getIterator() {
return SelectResultsIterator(m_structVector, shared_from_this());
}
-int32_t StructSetImpl::getFieldIndex(const char* fieldname) {
- std::map<std::string, int32_t>::iterator iter =
+int32_t StructSetImpl::getFieldIndex(const std::string& fieldname) {
+ const auto& iter =
m_fieldNameIndexMap.find(fieldname);
if (iter != m_fieldNameIndexMap.end()) {
return iter->second;
--- End diff --
We should clear out commented sources.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode-native pull request #107: GEODE-3019: Refactor Struct class
Posted by dgkimura <gi...@git.apache.org>.
Github user dgkimura commented on a diff in the pull request:
https://github.com/apache/geode-native/pull/107#discussion_r126173633
--- Diff: src/cppcache/include/geode/Struct.hpp ---
@@ -138,7 +138,7 @@ class CPPCACHE_EXPORT Struct : public Serializable {
* Returns the name of the field corresponding to the index number in the
* Struct
*/
- virtual const char* getFieldName(const int32_t index) const;
+ virtual const std::string* getFieldName(const int32_t index) const;
--- End diff --
Yeah, if we consider when field does not exist at index a failure case, then I agree that throwing is better than returning `nullptr` and expecting someone else to handle it or blow up later.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode-native pull request #107: GEODE-3019: Refactor Struct class
Posted by dgkimura <gi...@git.apache.org>.
Github user dgkimura commented on a diff in the pull request:
https://github.com/apache/geode-native/pull/107#discussion_r126027361
--- Diff: src/cppcache/include/geode/Struct.hpp ---
@@ -138,7 +138,7 @@ class CPPCACHE_EXPORT Struct : public Serializable {
* Returns the name of the field corresponding to the index number in the
* Struct
*/
- virtual const char* getFieldName(const int32_t index) const;
+ virtual const std::string* getFieldName(const int32_t index) const;
--- End diff --
Any reason why you're returning `std::string*` instead of `std::string` or `std::string&`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode-native pull request #107: GEODE-3019: Refactor Struct class
Posted by pivotal-jbarrett <gi...@git.apache.org>.
Github user pivotal-jbarrett commented on a diff in the pull request:
https://github.com/apache/geode-native/pull/107#discussion_r126116586
--- Diff: src/cppcache/integration-test/testThinClientRemoteQuerySS.cpp ---
@@ -56,59 +57,63 @@ const wchar_t* checkNullString(const wchar_t* str) {
return ((str == nullptr) ? L"(null)" : str);
}
+std::string checkNullString(const std::string* str) {
+ return ((str == nullptr) ? "(null)" : *str);
+}
+
void _printFields(CacheablePtr field, Struct* ssptr, int32_t& fields) {
if (auto portfolio = std::dynamic_pointer_cast<Portfolio>(field)) {
printf(" pulled %s :- ID %d, pkid %s\n",
- checkNullString(ssptr->getFieldName(fields)), portfolio->getID(),
+ checkNullString(ssptr->getFieldName(fields)).c_str(), portfolio->getID(),
--- End diff --
I think I would just rather see the tests not log so much crap and if they do that they use a logger with stream functionality.
```
if (log.debug) {
log.debug << "Doing some logging. somevar=" << someVar << ", someOtherVar=" << someOtherVar;
}
```
😒
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode-native pull request #107: GEODE-3019: Refactor Struct class
Posted by pivotal-jbarrett <gi...@git.apache.org>.
Github user pivotal-jbarrett commented on a diff in the pull request:
https://github.com/apache/geode-native/pull/107#discussion_r127312259
--- Diff: src/cppcache/src/StructSetImpl.cpp ---
@@ -78,13 +78,12 @@ int32_t StructSetImpl::getFieldIndex(const char* fieldname) {
}
}
-const char* StructSetImpl::getFieldName(int32_t index) {
- for (std::map<std::string, int32_t>::iterator iter =
- m_fieldNameIndexMap.begin();
- iter != m_fieldNameIndexMap.end(); ++iter) {
- if (iter->second == index) return iter->first.c_str();
+const std::string& StructSetImpl::getFieldName(int32_t index) {
+ for (const auto& iter : m_fieldNameIndexMap) {
+ if (iter.second == index) return (iter.first);
}
- return nullptr;
+
+ throw OutOfRangeException("Struct: fieldName not found.");
--- End diff --
I am more keen myself on using `std::exception`s rather than our own.
Thoughts?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode-native pull request #107: GEODE-3019: Refactor Struct class
Posted by dgkimura <gi...@git.apache.org>.
Github user dgkimura commented on a diff in the pull request:
https://github.com/apache/geode-native/pull/107#discussion_r128661186
--- Diff: src/cppcache/test/StructSetTest.cpp ---
@@ -0,0 +1,91 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include <gtest/gtest.h>
+#include <stdexcept>
+
+#include <StructSetImpl.hpp>
+
+using namespace apache::geode::client;
+
+TEST(StructSetTest, Basic) {
--- End diff --
Woot more unittests!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode-native pull request #107: GEODE-3019: Refactor Struct class
Posted by echobravopapa <gi...@git.apache.org>.
Github user echobravopapa closed the pull request at:
https://github.com/apache/geode-native/pull/107
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode-native pull request #107: GEODE-3019: Refactor Struct class
Posted by echobravopapa <gi...@git.apache.org>.
Github user echobravopapa commented on a diff in the pull request:
https://github.com/apache/geode-native/pull/107#discussion_r126029862
--- Diff: src/cppcache/include/geode/Struct.hpp ---
@@ -138,7 +138,7 @@ class CPPCACHE_EXPORT Struct : public Serializable {
* Returns the name of the field corresponding to the index number in the
* Struct
*/
- virtual const char* getFieldName(const int32_t index) const;
+ virtual const std::string* getFieldName(const int32_t index) const;
--- End diff --
Yes. To remain compatible with the existing usage of the method were `nullptr` is returned.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode-native pull request #107: GEODE-3019: Refactor Struct class
Posted by echobravopapa <gi...@git.apache.org>.
Github user echobravopapa commented on a diff in the pull request:
https://github.com/apache/geode-native/pull/107#discussion_r126030201
--- Diff: src/cppcache/integration-test/testThinClientRemoteQuerySS.cpp ---
@@ -56,59 +57,63 @@ const wchar_t* checkNullString(const wchar_t* str) {
return ((str == nullptr) ? L"(null)" : str);
}
+std::string checkNullString(const std::string* str) {
+ return ((str == nullptr) ? "(null)" : *str);
+}
+
void _printFields(CacheablePtr field, Struct* ssptr, int32_t& fields) {
if (auto portfolio = std::dynamic_pointer_cast<Portfolio>(field)) {
printf(" pulled %s :- ID %d, pkid %s\n",
- checkNullString(ssptr->getFieldName(fields)), portfolio->getID(),
+ checkNullString(ssptr->getFieldName(fields)).c_str(), portfolio->getID(),
--- End diff --
That is a good idea. There are 3 versions of checkNullString and it's test code so we minimized changes here in the tests...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode-native pull request #107: GEODE-3019: Refactor Struct class
Posted by echobravopapa <gi...@git.apache.org>.
Github user echobravopapa commented on a diff in the pull request:
https://github.com/apache/geode-native/pull/107#discussion_r127285623
--- Diff: src/cppcache/src/StructSetImpl.cpp ---
@@ -66,8 +66,8 @@ SelectResultsIterator StructSetImpl::getIterator() {
return SelectResultsIterator(m_structVector, shared_from_this());
}
-int32_t StructSetImpl::getFieldIndex(const char* fieldname) {
- std::map<std::string, int32_t>::iterator iter =
+int32_t StructSetImpl::getFieldIndex(const std::string& fieldname) {
--- End diff --
I'll have a look
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode-native pull request #107: GEODE-3019: Refactor Struct class
Posted by echobravopapa <gi...@git.apache.org>.
Github user echobravopapa commented on a diff in the pull request:
https://github.com/apache/geode-native/pull/107#discussion_r127285583
--- Diff: src/cppcache/src/StructSetImpl.cpp ---
@@ -66,8 +66,8 @@ SelectResultsIterator StructSetImpl::getIterator() {
return SelectResultsIterator(m_structVector, shared_from_this());
}
-int32_t StructSetImpl::getFieldIndex(const char* fieldname) {
- std::map<std::string, int32_t>::iterator iter =
+int32_t StructSetImpl::getFieldIndex(const std::string& fieldname) {
+ const auto& iter =
m_fieldNameIndexMap.find(fieldname);
if (iter != m_fieldNameIndexMap.end()) {
return iter->second;
--- End diff --
will do
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode-native pull request #107: GEODE-3019: Refactor Struct class
Posted by pivotal-jbarrett <gi...@git.apache.org>.
Github user pivotal-jbarrett commented on a diff in the pull request:
https://github.com/apache/geode-native/pull/107#discussion_r127278256
--- Diff: src/cppcache/src/StructSetImpl.cpp ---
@@ -66,8 +66,8 @@ SelectResultsIterator StructSetImpl::getIterator() {
return SelectResultsIterator(m_structVector, shared_from_this());
}
-int32_t StructSetImpl::getFieldIndex(const char* fieldname) {
- std::map<std::string, int32_t>::iterator iter =
+int32_t StructSetImpl::getFieldIndex(const std::string& fieldname) {
--- End diff --
should this method be a const method?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] geode-native pull request #107: GEODE-3019: Refactor Struct class
Posted by dgkimura <gi...@git.apache.org>.
Github user dgkimura commented on a diff in the pull request:
https://github.com/apache/geode-native/pull/107#discussion_r126027851
--- Diff: src/cppcache/include/geode/Struct.hpp ---
@@ -152,9 +152,7 @@ class CPPCACHE_EXPORT Struct : public Serializable {
Struct();
- typedef std::unordered_map<CacheableStringPtr, CacheableInt32Ptr,
- dereference_hash<CacheableStringPtr>,
- dereference_equal_to<CacheableStringPtr>>
--- End diff --
Nice!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---