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/09/14 23:33:48 UTC
[GitHub] [geode-native] pivotal-jbarrett opened pull request #353:
GEODE-5738: Fixes google-global-names-in-headers warning
[ Full content available at: https://github.com/apache/geode-native/pull/353 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #353:
GEODE-5738: Fixes google-global-names-in-headers warning
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
No-name namespace again. Looks like there are more of these, I'm gonna stop flagging them here.
[ Full content available at: https://github.com/apache/geode-native/pull/353 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] echobravopapa commented on pull request #353:
GEODE-5738: Fixes google-global-names-in-headers warning
Posted by "echobravopapa (GitHub)" <gi...@apache.org>.
hmmm, wonder why these aren't used anymore...
[ Full content available at: https://github.com/apache/geode-native/pull/353 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #353:
GEODE-5738: Fixes google-global-names-in-headers warning
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
Would `= default` work here?
[ Full content available at: https://github.com/apache/geode-native/pull/353 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #353:
GEODE-5738: Fixes google-global-names-in-headers warning
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
Pls remove commented code here and below
[ Full content available at: https://github.com/apache/geode-native/pull/353 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on pull request
#353: GEODE-5738: Fixes google-global-names-in-headers warning
Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
Yup!
[ Full content available at: https://github.com/apache/geode-native/pull/353 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #353:
GEODE-5738: Fixes google-global-names-in-headers warning
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
Do we have a lot of unused member functions in the test code? What's the extent of this problem? I'd hate to disable this warning if we don't have to...
[ Full content available at: https://github.com/apache/geode-native/pull/353 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on pull request
#353: GEODE-5738: Fixes google-global-names-in-headers warning
Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
These comments are logging statements that get uncommented when there is a problem with this tests. Given this isn't production code and to be deprecated tests that I am not going to touch logging comments. This would be a months long effort to get rid of this behavior in all the tests.
[ Full content available at: https://github.com/apache/geode-native/pull/353 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on pull request
#353: GEODE-5738: Fixes google-global-names-in-headers warning
Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
Probably
[ Full content available at: https://github.com/apache/geode-native/pull/353 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #353:
GEODE-5738: Fixes google-global-names-in-headers warning
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
GEODE-5755 filed for tracking
[ Full content available at: https://github.com/apache/geode-native/pull/353 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on issue #353:
GEODE-5738: Fixes google-global-names-in-headers warning
Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
@pdxcodemonkey you good?
[ Full content available at: https://github.com/apache/geode-native/pull/353 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] moleske commented on pull request #353:
GEODE-5738: Fixes google-global-names-in-headers warning
Posted by "moleske (GitHub)" <gi...@apache.org>.
The default in CLion is to put the `*` on the variable rather than the type. I think CLion supports integrated Clang but haven't looked in a while. See [this](https://stackoverflow.com/questions/45656445/clion-formatting-of-pointer-references) for changing those settings.
[ Full content available at: https://github.com/apache/geode-native/pull/353 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on pull request
#353: GEODE-5738: Fixes google-global-names-in-headers warning
Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
They don't but this was an auto format change.
[ Full content available at: https://github.com/apache/geode-native/pull/353 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #353:
GEODE-5738: Fixes google-global-names-in-headers warning
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
(Holding my nose while I approve this). I understand your reasoning, and I know you're right about the effort required, but continuing to kick the can down the road on fixing all the bad test code is frustrating.
[ Full content available at: https://github.com/apache/geode-native/pull/353 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] echobravopapa commented on pull request #353:
GEODE-5738: Fixes google-global-names-in-headers warning
Posted by "echobravopapa (GitHub)" <gi...@apache.org>.
CLion is wrong.
[ Full content available at: https://github.com/apache/geode-native/pull/353 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on pull request
#353: GEODE-5738: Fixes google-global-names-in-headers warning
Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
CLion lacks the ability to actually apply the Google rule here. They flipped a coin and chose right alignment. We mostly have left alignment. The google rule is either but be consistent. Clang-format will determine the majority in the file and correct the minority.
[ Full content available at: https://github.com/apache/geode-native/pull/353 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #353:
GEODE-5738: Fixes google-global-names-in-headers warning
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
Namespace with no name again
[ Full content available at: https://github.com/apache/geode-native/pull/353 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #353:
GEODE-5738: Fixes google-global-names-in-headers warning
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
I'm not familiar with this construct. An anonymous namespace? Was this intentional?
[ Full content available at: https://github.com/apache/geode-native/pull/353 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on pull request
#353: GEODE-5738: Fixes google-global-names-in-headers warning
Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
Yup! Its a technique to isolate otherwise global namespace `using` or type definitions to the current compilation unit.
[ Full content available at: https://github.com/apache/geode-native/pull/353 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on pull request
#353: GEODE-5738: Fixes google-global-names-in-headers warning
Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
Yes! There are several headers that are included that implement lots of methods that are never used in some units. If these were compiled separately as a library the issue would go away but that is way beyond the point of this change right now. The goal is to get the production code clear. So for now we leave the old test framework bad.
[ Full content available at: https://github.com/apache/geode-native/pull/353 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #353:
GEODE-5738: Fixes google-global-names-in-headers warning
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
... and again
[ Full content available at: https://github.com/apache/geode-native/pull/353 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on pull request
#353: GEODE-5738: Fixes google-global-names-in-headers warning
Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
These comments are logging statements that get uncommented when there is a problem with this tests. Given this isn't production code and to be deprecated tests that I am not going to touch logging comments.
[ Full content available at: https://github.com/apache/geode-native/pull/353 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #353:
GEODE-5738: Fixes google-global-names-in-headers warning
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
Guess we'll get to this when we refactor all the test code anyway...
[ Full content available at: https://github.com/apache/geode-native/pull/353 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #353:
GEODE-5738: Fixes google-global-names-in-headers warning
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
Should we be consistent with the test namespace? I'm not certain there's a significant reason to have both apache::geode::testing and apache::geode::client::testing. Seems to me the one including client is the right way to go, since we're not supposed to be testing geode itself.
[ Full content available at: https://github.com/apache/geode-native/pull/353 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] echobravopapa commented on pull request #353:
GEODE-5738: Fixes google-global-names-in-headers warning
Posted by "echobravopapa (GitHub)" <gi...@apache.org>.
do we have dueling auto-formatting ? seems like this would have been caught before
[ Full content available at: https://github.com/apache/geode-native/pull/353 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] echobravopapa commented on pull request #353:
GEODE-5738: Fixes google-global-names-in-headers warning
Posted by "echobravopapa (GitHub)" <gi...@apache.org>.
Sure be nice to get rid of these Helper classes...
[ Full content available at: https://github.com/apache/geode-native/pull/353 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #353:
GEODE-5738: Fixes google-global-names-in-headers warning
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
These comments add no value, let's get rid of em
[ Full content available at: https://github.com/apache/geode-native/pull/353 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #353:
GEODE-5738: Fixes google-global-names-in-headers warning
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
GEODE-5756 filed for tracking
[ Full content available at: https://github.com/apache/geode-native/pull/353 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey closed pull request #353:
GEODE-5738: Fixes google-global-names-in-headers warning
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
[ pull request closed by pdxcodemonkey ]
[ Full content available at: https://github.com/apache/geode-native/pull/353 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #353:
GEODE-5738: Fixes google-global-names-in-headers warning
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
Probably don't need this comment
[ Full content available at: https://github.com/apache/geode-native/pull/353 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode-native] pdxcodemonkey commented on pull request #353:
GEODE-5738: Fixes google-global-names-in-headers warning
Posted by "pdxcodemonkey (GitHub)" <gi...@apache.org>.
Pls remove commented code
[ Full content available at: https://github.com/apache/geode-native/pull/353 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org