You are viewing a plain text version of this content. The canonical link for it is here.

Posted to notifications@geode.apache.org by "pivotal-jbarrett (GitHub)" <gi...@apache.org> on 2018/11/09 22:59:28 UTC

[GitHub] [geode-native] pivotal-jbarrett opened pull request #401: GEODE-2484: Replace ACE Map with synchronized unordered_map

* Adds an synchronized_map wrapper template to synchronize a map with a
  mutex.

* Replace unsynchronized ACE Map with std::unordered_map.

[ Full content available at: https://github.com/apache/geode-native/pull/401 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode-native] pdxcodemonkey commented on pull request #401: GEODE-2484: Replace ACE Map with synchronized unordered_map

Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.

What do these names mean?  It'd be really helpful to rename these to provide some context...

[ Full content available at: https://github.com/apache/geode-native/pull/401 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode-native] pivotal-jbarrett commented on issue #401: GEODE-2484: Replace ACE Map with synchronized unordered_map

Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.

@pdxcodemonkey Ready for final review.

[ Full content available at: https://github.com/apache/geode-native/pull/401 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode-native] pivotal-jbarrett commented on pull request #401: GEODE-2484: Replace ACE Map with synchronized unordered_map

Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.

Geez guys, focus on the lines I actually change not just lines that get formatting changes! Good find though.

[ Full content available at: https://github.com/apache/geode-native/pull/401 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode-native] pivotal-jbarrett commented on pull request #401: GEODE-2484: Replace ACE Map with synchronized unordered_map

Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.

I feel like`kv` is fairly common usage for iterating through a map structure in C++. 

I’m open to suggestions.


[ Full content available at: https://github.com/apache/geode-native/pull/401 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode-native] pdxcodemonkey commented on pull request #401: GEODE-2484: Replace ACE Map with synchronized unordered_map

Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.

Okay, I think you get the picture about short variable names...

[ Full content available at: https://github.com/apache/geode-native/pull/401 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode-native] pivotal-jbarrett commented on pull request #401: GEODE-2484: Replace ACE Map with synchronized unordered_map

Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.

I’m not changing all global search and replaced lines bad variable names. They will happen if I’m actually manually editing something.

[ Full content available at: https://github.com/apache/geode-native/pull/401 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode-native] pdxcodemonkey commented on pull request #401: GEODE-2484: Replace ACE Map with synchronized unordered_map

Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.

Could use a more descriptive name than `kv`

[ Full content available at: https://github.com/apache/geode-native/pull/401 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode-native] pdxcodemonkey commented on pull request #401: GEODE-2484: Replace ACE Map with synchronized unordered_map

Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.

Lambda - fancy!

[ Full content available at: https://github.com/apache/geode-native/pull/401 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode-native] pdxcodemonkey commented on pull request #401: GEODE-2484: Replace ACE Map with synchronized unordered_map

Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.

These variable names are bad...

[ Full content available at: https://github.com/apache/geode-native/pull/401 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode-native] pdxcodemonkey commented on issue #401: DON'T REVIEW: GEODE-2484: Replace ACE Map with synchronized unordered_map

Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.

I'm happy to approve this PR after you let me know what you want to do about addressing the namespace issues pointed out in SerializationRegistry.cpp.  Thanks!

[ Full content available at: https://github.com/apache/geode-native/pull/401 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode-native] pdxcodemonkey commented on pull request #401: GEODE-2484: Replace ACE Map with synchronized unordered_map

Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.

NNnnnoooooooooooo!  Actually think this is vestigial from some earlier change you made or something.  We removed it and built successfully.

[ Full content available at: https://github.com/apache/geode-native/pull/401 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode-native] pdxcodemonkey commented on pull request #401: GEODE-2484: Replace ACE Map with synchronized unordered_map

Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.

endPoint?

[ Full content available at: https://github.com/apache/geode-native/pull/401 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode-native] pivotal-jbarrett commented on pull request #401: GEODE-2484: Replace ACE Map with synchronized unordered_map

Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.

Yup, global search and replace for iterative access. Not changing random bad variable names.

[ Full content available at: https://github.com/apache/geode-native/pull/401 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode-native] pdxcodemonkey commented on pull request #401: GEODE-2484: Replace ACE Map with synchronized unordered_map

Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.

Just use `clientMetadataService` variable here

[ Full content available at: https://github.com/apache/geode-native/pull/401 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode-native] pdxcodemonkey commented on pull request #401: GEODE-2484: Replace ACE Map with synchronized unordered_map

Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.

DSFid here is not qualified by the internal:: namespace.  I don't know where you're picking this up, but usages of DSCode and DSFid should probably all agree here and either all use the namespace or all not.

[ Full content available at: https://github.com/apache/geode-native/pull/401 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode-native] pivotal-jbarrett commented on pull request #401: GEODE-2484: Replace ACE Map with synchronized unordered_map

Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.

There are ACE types in this header. I think it failed under Windows when the header was removed from other files. This file was piggy backing off another.

[ Full content available at: https://github.com/apache/geode-native/pull/401 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode-native] pivotal-jbarrett closed pull request #401: GEODE-2484: Replace ACE Map with synchronized unordered_map

Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.

[ pull request closed by pivotal-jbarrett ]

[ Full content available at: https://github.com/apache/geode-native/pull/401 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org