You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by "moleske (GitHub)" <gi...@apache.org> on 2019/10/26 00:11:22 UTC
[GitHub] [geode-native] moleske opened pull request #543: Some
cleanup in ThinClientPoolDM
LGTM pointed out a few things, tried to fix them. Also cleaned up some thing CLion pointed out to me. Hopefully this doesn't add new things to LGTM
Will make a jira later if this turns out alright, this was mostly stuff I could easily pick up/put down while waiting for some other builds
[ Full content available at: https://github.com/apache/geode-native/pull/543 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on pull request
#543: Some cleanup in ThinClientPoolDM
Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
Same issue found in a few other files.
[ Full content available at: https://github.com/apache/geode-native/pull/543 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] lgtm-com[bot] commented on issue #543: Some
cleanup in ThinClientPoolDM
Posted by lg...@gitbox.apache.org.
This pull request **fixes 3 alerts** when merging 68afb263c922e891a886268f6b1f2caa94af0c4d into 3a51a80c27c4e811526eb489a305f08647381955 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode-native/rev/pr-6f83d62f9637c990b9c2892dfdf50787d6a16d5f)
**fixed alerts:**
* 2 for Empty branch of conditional
* 1 for Declaration hides parameter
[ Full content available at: https://github.com/apache/geode-native/pull/543 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on pull request
#543: Some cleanup in ThinClientPoolDM
Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
Move the comment so that the `.empty()` isn't line split.
[ Full content available at: https://github.com/apache/geode-native/pull/543 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] moleske closed pull request #543: Some
cleanup in ThinClientPoolDM
Posted by "moleske (GitHub)" <gi...@apache.org>.
[ pull request closed by moleske ]
[ Full content available at: https://github.com/apache/geode-native/pull/543 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #543:
Some cleanup in ThinClientPoolDM
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
How about we get rid of `err` here altogether? Checking it against GF_NOERR then doing `throwExceptionIfError` when we already know it's an error seems redundant...
```
throwExceptionIfError("Operation Failed", sendSyncRequest(request, reply));
```
[ Full content available at: https://github.com/apache/geode-native/pull/543 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on pull request
#543: Some cleanup in ThinClientPoolDM
Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
`auto`
[ Full content available at: https://github.com/apache/geode-native/pull/543 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] dgkimura commented on pull request #543: Some
cleanup in ThinClientPoolDM
Posted by "dgkimura (GitHub)" <gi...@apache.org>.
I agree that an empty semicolon is silly. Again though, as someone who has been heavily tainted by C, I'd suggest altering the macro to be do/while(0) rather than deleting the semicolon at all the call sites. Besides the aesthetic appeal of seeing statements terminated by semicolon, it would reduce the diff if a future change moved the logic into a function by the same name.
[ Full content available at: https://github.com/apache/geode-native/pull/543 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] moleske commented on pull request #543: Some
cleanup in ThinClientPoolDM
Posted by "moleske (GitHub)" <gi...@apache.org>.
Clang-tidy in CLion said it was `Redundant void argument list in function definition`. My understanding was C++ didn't allow the C example you shared. I didn't clean up the header file, it still has `(void)` with the `Redundant void argument` message
[ Full content available at: https://github.com/apache/geode-native/pull/543 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on pull request
#543: Some cleanup in ThinClientPoolDM
Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
There are several things wrong with this line.
1) `auto*` really isn't a thing.
2) a `new` doesn't need to be "copied" into a pointer.
3) Since the `TcrChunkedResult` doesn't really survive outside the life of this method or the reply object it should probably be a `std::shared_ptr` owned by both this method and the reply object or maybe even a `std::unique_ptr` owned by either the method or the reply object. Either way it removes the explicit deletes in multiple places in this method and nothing can leak.
Simple fix:
```C++
auto resultProcessor = std::unique_ptr<ChunkedFunctionExecutionResponse>(
new ChunkedFunctionExecutionResponse(reply, (m_getResult & 2) == 2, *m_rc,
m_resultCollectorLock));
reply.setChunkedResultHandler(resultProcessor.get());
```
and delete all:
```C++
delete resultProcessor;
resultProcessor = nullptr;
```
[ Full content available at: https://github.com/apache/geode-native/pull/543 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] lgtm-com[bot] commented on issue #543: Some
cleanup in ThinClientPoolDM
Posted by lg...@gitbox.apache.org.
This pull request **fixes 3 alerts** when merging ddfc8a6dda9f9f60812d989202bdf811b9465389 into 3a51a80c27c4e811526eb489a305f08647381955 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode-native/rev/pr-a0a46192f73e874cf2ee324bec9d8253c3f8ee49)
**fixed alerts:**
* 2 for Empty branch of conditional
* 1 for Declaration hides parameter
[ Full content available at: https://github.com/apache/geode-native/pull/543 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] dgkimura commented on pull request #543: Some
cleanup in ThinClientPoolDM
Posted by "dgkimura (GitHub)" <gi...@apache.org>.
Admittedly, I don't know the current best-practice in C++, but I think this looks a little meh and leads me think the macro is wrong. If the spurious semicolon bothers you, then I would change the macro to...
```
#define _GEODE_SAFE_DELETE(x) \
do { \
delete x; \
x = nullptr; \
} while (0)
```
[ Full content available at: https://github.com/apache/geode-native/pull/543 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] moleske commented on pull request #543: Some
cleanup in ThinClientPoolDM
Posted by "moleske (GitHub)" <gi...@apache.org>.
this was default settings in CLion, and me hitting `option+enter` as fast I could, so didn't look that closely at the change. I can change it back pretty easily (and don't have strong feelings about it). Strangely enough CLion considered it error with the note `Empty expression statement has no effect; remove unncessary ';' to silence this warning` 🤷♀️
[ Full content available at: https://github.com/apache/geode-native/pull/543 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] lgtm-com[bot] commented on issue #543: Some
cleanup in ThinClientPoolDM
Posted by lg...@gitbox.apache.org.
This pull request **fixes 3 alerts** when merging a0e59a5b33576aa3dc272b15f5b1451d4462cf9f into 3a51a80c27c4e811526eb489a305f08647381955 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode-native/rev/pr-1eae5e047896d941f1619c9608db136d613df8b7)
**fixed alerts:**
* 2 for Empty branch of conditional
* 1 for Declaration hides parameter
[ Full content available at: https://github.com/apache/geode-native/pull/543 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] moleske commented on pull request #543: Some
cleanup in ThinClientPoolDM
Posted by "moleske (GitHub)" <gi...@apache.org>.
For closure, there was a warning in the editor for missing override that I didn't notice while trying to resolve the ton of other messages. Proves that a 4th pass at the warnings would've helped
[ Full content available at: https://github.com/apache/geode-native/pull/543 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org