You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by PivotalSarge <gi...@git.apache.org> on 2017/02/15 21:42:06 UTC
[GitHub] geode-native pull request #13: GEODE-2476: Replace gfcpp with geode.
GitHub user PivotalSarge opened a pull request:
https://github.com/apache/geode-native/pull/13
GEODE-2476: Replace gfcpp with geode.
- Rename directories and files with gfcpp into their name to
instead use geode and update all references thereto.
- Rename the gfcpp executable to apache-geode-getversion and
modify it to print the version even in the absence of
command-line arguments.
- Rename gfcpp.properties to geode.properties and update all
references thereto.
- Ensure formatting style guide compliance.
- Re-applying fixes for Windows compilation errors.
- Fix logic for using clang tidy auto-fix.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/PivotalSarge/geode-native feature/GEODE-2476
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/geode-native/pull/13.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 #13
----
commit 56e21e5e89bed28416237c6d752d60f9d894e9b7
Author: Sarge <md...@pivotal.io>
Date: 2017-02-14T18:31:51Z
GEODE-2476: Replace gfcpp with geode.
- Rename directories and files with gfcpp into their name to
instead use geode and update all references thereto.
- Rename the gfcpp executable to apache-geode-getversion and
modify it to print the version even in the absence of
command-line arguments.
- Rename gfcpp.properties to geode.properties and update all
references thereto.
- Ensure formatting style guide compliance.
- Re-applying fixes for Windows compilation errors.
- Fix logic for using clang tidy auto-fix.
----
---
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 #13: GEODE-2476: Replace gfcpp with geode.
Posted by PivotalSarge <gi...@git.apache.org>.
Github user PivotalSarge commented on a diff in the pull request:
https://github.com/apache/geode-native/pull/13#discussion_r101796694
--- Diff: src/CMakeLists.txt ---
@@ -224,7 +222,7 @@ add_subdirectory(cppcache)
add_subdirectory(cryptoimpl)
add_subdirectory(dhimpl)
add_subdirectory(sqliteimpl)
-add_subdirectory(gfcpp)
+add_subdirectory(getversion)
--- End diff --
I'm totally cool with deleting it. I didn't because it's been part of the install; some users may rely on it to determine version?
---
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 #13: GEODE-2476: Replace gfcpp with geode.
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/13#discussion_r101659601
--- Diff: src/cppcache/include/geode/AttributesFactory.hpp ---
@@ -1,7 +1,7 @@
#pragma once
-#ifndef GEODE_GFCPP_ATTRIBUTESFACTORY_H_
-#define GEODE_GFCPP_ATTRIBUTESFACTORY_H_
+#ifndef GEODE_GEODE_ATTRIBUTESFACTORY_H_
--- End diff --
Really? GEODE_GEODE doesn't seem right?!
---
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 #13: GEODE-2476: Replace gfcpp with geode.
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/geode-native/pull/13
---
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 #13: GEODE-2476: Replace gfcpp with geode.
Posted by PivotalSarge <gi...@git.apache.org>.
Github user PivotalSarge commented on a diff in the pull request:
https://github.com/apache/geode-native/pull/13#discussion_r101796329
--- Diff: src/cppcache/include/geode/AttributesFactory.hpp ---
@@ -20,7 +20,7 @@
* limitations under the License.
*/
-#include "gfcpp_globals.hpp"
+#include "geode_globals.hpp"
#include "gf_types.hpp"
--- End diff --
That's a separate JIRA ticket and hence will be a separate (upcoming) PR.
---
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 #13: GEODE-2476: Replace gfcpp with geode.
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/13#discussion_r101824812
--- Diff: src/CMakeLists.txt ---
@@ -224,7 +222,7 @@ add_subdirectory(cppcache)
add_subdirectory(cryptoimpl)
add_subdirectory(dhimpl)
add_subdirectory(sqliteimpl)
-add_subdirectory(gfcpp)
+add_subdirectory(getversion)
--- End diff --
If they are relying on it then their scripts will break either way. There are better ways to get version. I don't think I have ever seen anyone anywhere use it. I don't even see it in our documents. I say kill it!
---
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 #13: GEODE-2476: Replace gfcpp with geode.
Posted by mmartell <gi...@git.apache.org>.
Github user mmartell commented on a diff in the pull request:
https://github.com/apache/geode-native/pull/13#discussion_r101783052
--- Diff: src/cppcache/include/geode/AttributesFactory.hpp ---
@@ -20,7 +20,7 @@
* limitations under the License.
*/
-#include "gfcpp_globals.hpp"
+#include "geode_globals.hpp"
#include "gf_types.hpp"
--- End diff --
+1 for geode_types.hpp
---
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 #13: GEODE-2476: Replace gfcpp with geode.
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/13#discussion_r101659366
--- Diff: src/CMakeLists.txt ---
@@ -224,7 +222,7 @@ add_subdirectory(cppcache)
add_subdirectory(cryptoimpl)
add_subdirectory(dhimpl)
add_subdirectory(sqliteimpl)
-add_subdirectory(gfcpp)
+add_subdirectory(getversion)
--- End diff --
Eeeek! This is the best name we could come up with for this stupid util? May we should just delete it?
---
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.
---
Re: [GitHub] geode-native pull request #13: GEODE-2476: Replace gfcpp
with geode.
Posted by Michael William Dodge <md...@pivotal.io>.
I've modified PR #13 to remove the vestigial executable and to elide the duplicate GEODE_ in the legacy include guards.
Sarge
> On 17 Feb, 2017, at 11:06, pivotal-jbarrett <gi...@git.apache.org> wrote:
>
> Github user pivotal-jbarrett commented on a diff in the pull request:
>
> https://github.com/apache/geode-native/pull/13#discussion_r101825168
>
> --- Diff: src/cppcache/include/geode/AttributesFactory.hpp ---
> @@ -1,7 +1,7 @@
> #pragma once
>
> -#ifndef GEODE_GFCPP_ATTRIBUTESFACTORY_H_
> -#define GEODE_GFCPP_ATTRIBUTESFACTORY_H_
> +#ifndef GEODE_GEODE_ATTRIBUTESFACTORY_H_
> --- End diff --
>
> I see an argument that `include/geode/` is the root for the public headers, therefore the guards should be `GEODE_ATTRIBUTESFACTORY_H_`.
>
>
> ---
> 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 #13: GEODE-2476: Replace gfcpp with geode.
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/13#discussion_r101825168
--- Diff: src/cppcache/include/geode/AttributesFactory.hpp ---
@@ -1,7 +1,7 @@
#pragma once
-#ifndef GEODE_GFCPP_ATTRIBUTESFACTORY_H_
-#define GEODE_GFCPP_ATTRIBUTESFACTORY_H_
+#ifndef GEODE_GEODE_ATTRIBUTESFACTORY_H_
--- End diff --
I see an argument that `include/geode/` is the root for the public headers, therefore the guards should be `GEODE_ATTRIBUTESFACTORY_H_`.
---
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 #13: GEODE-2476: Replace gfcpp with geode.
Posted by mmartell <gi...@git.apache.org>.
Github user mmartell commented on a diff in the pull request:
https://github.com/apache/geode-native/pull/13#discussion_r101782908
--- Diff: src/cppcache/include/geode/AttributesFactory.hpp ---
@@ -1,7 +1,7 @@
#pragma once
-#ifndef GEODE_GFCPP_ATTRIBUTESFACTORY_H_
-#define GEODE_GFCPP_ATTRIBUTESFACTORY_H_
+#ifndef GEODE_GEODE_ATTRIBUTESFACTORY_H_
--- End diff --
+1 for GEODE_ATTRIBUTES_FACTORY_H_
---
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 #13: GEODE-2476: Replace gfcpp with geode.
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/13#discussion_r101659693
--- Diff: src/cppcache/include/geode/AttributesFactory.hpp ---
@@ -20,7 +20,7 @@
* limitations under the License.
*/
-#include "gfcpp_globals.hpp"
+#include "geode_globals.hpp"
#include "gf_types.hpp"
--- End diff --
Why not gf_types.hpp renamed to geode_types.hpp?
---
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 #13: GEODE-2476: Replace gfcpp with geode.
Posted by PivotalSarge <gi...@git.apache.org>.
Github user PivotalSarge commented on a diff in the pull request:
https://github.com/apache/geode-native/pull/13#discussion_r101796462
--- Diff: src/cppcache/include/geode/AttributesFactory.hpp ---
@@ -1,7 +1,7 @@
#pragma once
-#ifndef GEODE_GFCPP_ATTRIBUTESFACTORY_H_
-#define GEODE_GFCPP_ATTRIBUTESFACTORY_H_
+#ifndef GEODE_GEODE_ATTRIBUTESFACTORY_H_
--- End diff --
Google specification is project name, directory name, file name.
---
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.
---