You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hawq.apache.org by xunzhang <gi...@git.apache.org> on 2016/12/03 08:24:22 UTC
[GitHub] incubator-hawq pull request #1036: HAWQ-1188. Fix "-Wtautological-constant-o...
GitHub user xunzhang opened a pull request:
https://github.com/apache/incubator-hawq/pull/1036
HAWQ-1188. Fix "-Wtautological-constant-out-of-range-compare, -Wtautological-compare, -Wpointer-bool-conversion, -Wnon-literal-null-conversion, -Wincompatible-pointer-types, -Wincompatible-pointer-types-discards-qualifiers, -Wlogical-not-parentheses" compile warnings under osx.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/xunzhang/incubator-hawq HAWQ-1188
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-hawq/pull/1036.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 #1036
----
commit 47361ae71f8c90930c68985245aeef669437574b
Author: xunzhang <xu...@gmail.com>
Date: 2016-12-03T08:14:10Z
HAWQ-1188. Fix "-Wtautological-constant-out-of-range-compare, -Wtautological-compare, -Wpointer-bool-conversion, -Wnon-literal-null-conversion, -Wincompatible-pointer-types, -Wincompatible-pointer-types-discards-qualifiers, -Wlogical-not-parentheses" compile warnings under osx.
----
---
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] incubator-hawq pull request #1036: HAWQ-1188. Fix "-Wtautological-constant-o...
Posted by paul-guo- <gi...@git.apache.org>.
Github user paul-guo- commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/1036#discussion_r90804960
--- Diff: src/backend/resourcemanager/utils/simplestring.c ---
@@ -269,9 +269,9 @@ int SimpleStringToUInt32(SimpStringPtr str, uint32_t *value)
}
*/
-int SimpleStringToInt64(SimpStringPtr str, int64_t *value)
+int SimpleStringToInt64(SimpStringPtr str, int64 *value)
--- End diff --
See previous comments.
---
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] incubator-hawq pull request #1036: HAWQ-1188. Fix "-Wtautological-constant-o...
Posted by xunzhang <gi...@git.apache.org>.
Github user xunzhang commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/1036#discussion_r90876775
--- Diff: src/backend/gp_libpq_fe/fe-connect.c ---
@@ -436,7 +436,7 @@ PQconnectStartParams(const char **keywords,
{
conn->status = CONNECTION_BAD;
/* errorMessage is already set */
- return false;
+ return conn;
--- End diff --
I found gpdb has already fixed this line of code: https://github.com/xunzhang/incubator-hawq/blob/47361ae71f8c90930c68985245aeef669437574b/src/interfaces/libpq/fe-connect.c#L449.
---
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] incubator-hawq pull request #1036: HAWQ-1188. Fix "-Wtautological-constant-o...
Posted by paul-guo- <gi...@git.apache.org>.
Github user paul-guo- commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/1036#discussion_r90804848
--- Diff: src/backend/gp_libpq_fe/fe-exec.c ---
@@ -2992,7 +2992,6 @@ PQoidValue(const PGresult *res)
unsigned long result;
if (!res ||
--- End diff --
See comment above.
---
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] incubator-hawq pull request #1036: HAWQ-1188. Fix "-Wtautological-constant-o...
Posted by paul-guo- <gi...@git.apache.org>.
Github user paul-guo- commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/1036#discussion_r91224732
--- Diff: src/backend/resourcemanager/include/utils/simplestring.h ---
@@ -63,7 +63,7 @@ bool SimpleStringEmpty(SimpStringPtr str);
int SimpleStringLocateChar(SimpStringPtr str, char target, int *location);
/* string to the other number values. */
int SimpleStringToInt32(SimpStringPtr str, int32_t *value);
-int SimpleStringToInt64(SimpStringPtr str, int64_t *value);
+int SimpleStringToInt64(SimpStringPtr str, int64 *value);
--- End diff --
OK. Defining int64 as "long int" is really buggy assume it means a 64bit integer. Does this comes from pg or gpdb? I do not know why we have both int64 and int64_t.
---
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] incubator-hawq pull request #1036: HAWQ-1188. Fix "-Wtautological-constant-o...
Posted by xunzhang <gi...@git.apache.org>.
Github user xunzhang commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/1036#discussion_r91325539
--- Diff: src/backend/resourcemanager/include/utils/simplestring.h ---
@@ -63,7 +63,7 @@ bool SimpleStringEmpty(SimpStringPtr str);
int SimpleStringLocateChar(SimpStringPtr str, char target, int *location);
/* string to the other number values. */
int SimpleStringToInt32(SimpStringPtr str, int32_t *value);
-int SimpleStringToInt64(SimpStringPtr str, int64_t *value);
+int SimpleStringToInt64(SimpStringPtr str, int64 *value);
--- End diff --
I don't know either...
---
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] incubator-hawq pull request #1036: HAWQ-1188. Fix "-Wtautological-constant-o...
Posted by paul-guo- <gi...@git.apache.org>.
Github user paul-guo- commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/1036#discussion_r91224790
--- Diff: src/backend/cdb/cdbgang.c ---
@@ -444,7 +444,7 @@ disconnectAndDestroyGang(Gang *gp)
elog(DEBUG2, "Warning: disconnectAndDestroyGang called on an %s gang",
gp->active ? "active" : "allocated");
- if (gp->gang_id < 1 || gp->gang_id > 100000000 || gp->type > 10 || gp->size > 100000)
+ if (gp->gang_id < 1 || gp->gang_id > 100000000 || gp->type > GANGTYPE_PRIMARY_WRITER || gp->size > 100000)
--- End diff --
I mean gp-type < GANGTYPE_MIN.
---
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] incubator-hawq issue #1036: HAWQ-1188. Fix "-Wtautological-constant-out-of-r...
Posted by xunzhang <gi...@git.apache.org>.
Github user xunzhang commented on the issue:
https://github.com/apache/incubator-hawq/pull/1036
also cc @stanlyxiang
---
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] incubator-hawq issue #1036: HAWQ-1188. Fix "-Wtautological-constant-out-of-r...
Posted by xunzhang <gi...@git.apache.org>.
Github user xunzhang commented on the issue:
https://github.com/apache/incubator-hawq/pull/1036
To conclude, I think there needs no modification according to @paul-guo- 's review. Please check out my replies and review this pull request again @paul-guo- . Thanks!
---
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] incubator-hawq issue #1036: HAWQ-1188. Fix "-Wtautological-constant-out-of-r...
Posted by xunzhang <gi...@git.apache.org>.
Github user xunzhang commented on the issue:
https://github.com/apache/incubator-hawq/pull/1036
Merged into master.
---
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] incubator-hawq pull request #1036: HAWQ-1188. Fix "-Wtautological-constant-o...
Posted by xunzhang <gi...@git.apache.org>.
Github user xunzhang commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/1036#discussion_r90877708
--- Diff: src/interfaces/libpq/fe-exec.c ---
@@ -2386,7 +2386,7 @@ PQresultStatus(const PGresult *res)
char *
PQresStatus(ExecStatusType status)
{
- if (status < 0 || status >= sizeof pgresStatus / sizeof pgresStatus[0])
+ if (status < PGRES_EMPTY_QUERY || (unsigned int) status >= sizeof pgresStatus / sizeof pgresStatus[0])
--- End diff --
Ditto
---
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] incubator-hawq pull request #1036: HAWQ-1188. Fix "-Wtautological-constant-o...
Posted by paul-guo- <gi...@git.apache.org>.
Github user paul-guo- commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/1036#discussion_r90804840
--- Diff: src/backend/gp_libpq_fe/fe-exec.c ---
@@ -2968,7 +2968,7 @@ PQoidStatus(const PGresult *res)
size_t len;
- if (!res || !res->cmdStatus || strncmp(res->cmdStatus, "INSERT ", 7) != 0)
+ if (!res || strncmp(res->cmdStatus, "INSERT ", 7) != 0)
--- End diff --
I do not think assuming strncpy(NULL, ...) is ok is a good idea. At least I quickly searched on stackoverflow,
http://stackoverflow.com/questions/21865041/in-c-what-exactly-happens-when-you-pass-a-null-pointer-to-strcmp
Although this page is probably not accurate, but your change might risk not working in some lib/platform implementation.
---
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] incubator-hawq pull request #1036: HAWQ-1188. Fix "-Wtautological-constant-o...
Posted by xunzhang <gi...@git.apache.org>.
Github user xunzhang commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/1036#discussion_r90877631
--- Diff: src/backend/gp_libpq_fe/fe-exec.c ---
@@ -2992,7 +2992,6 @@ PQoidValue(const PGresult *res)
unsigned long result;
if (!res ||
--- End diff --
Ditto
---
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] incubator-hawq issue #1036: HAWQ-1188. Fix "-Wtautological-constant-out-of-r...
Posted by stanlyxiang <gi...@git.apache.org>.
Github user stanlyxiang commented on the issue:
https://github.com/apache/incubator-hawq/pull/1036
+1
---
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] incubator-hawq pull request #1036: HAWQ-1188. Fix "-Wtautological-constant-o...
Posted by xunzhang <gi...@git.apache.org>.
Github user xunzhang commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/1036#discussion_r90877683
--- Diff: src/interfaces/libpq/fe-connect.c ---
@@ -446,7 +446,7 @@ PQconnectStartParams(const char **keywords,
{
conn->status = CONNECTION_BAD;
/* errorMessage is already set */
- return false;
+ return conn;
--- End diff --
Ditto
---
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] incubator-hawq pull request #1036: HAWQ-1188. Fix "-Wtautological-constant-o...
Posted by paul-guo- <gi...@git.apache.org>.
Github user paul-guo- commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/1036#discussion_r90805120
--- Diff: src/interfaces/libpq/fe-exec.c ---
@@ -2710,7 +2710,6 @@ PQoidValue(const PGresult *res)
unsigned long result;
if (!res ||
--- End diff --
See previous comments.
---
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] incubator-hawq pull request #1036: HAWQ-1188. Fix "-Wtautological-constant-o...
Posted by paul-guo- <gi...@git.apache.org>.
Github user paul-guo- commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/1036#discussion_r90805089
--- Diff: src/interfaces/libpq/fe-connect.c ---
@@ -446,7 +446,7 @@ PQconnectStartParams(const char **keywords,
{
conn->status = CONNECTION_BAD;
/* errorMessage is already set */
- return false;
+ return conn;
--- End diff --
See comments as before.
---
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] incubator-hawq pull request #1036: HAWQ-1188. Fix "-Wtautological-constant-o...
Posted by xunzhang <gi...@git.apache.org>.
Github user xunzhang commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/1036#discussion_r90880247
--- Diff: src/backend/resourcemanager/include/utils/simplestring.h ---
@@ -63,7 +63,7 @@ bool SimpleStringEmpty(SimpStringPtr str);
int SimpleStringLocateChar(SimpStringPtr str, char target, int *location);
/* string to the other number values. */
int SimpleStringToInt32(SimpStringPtr str, int32_t *value);
-int SimpleStringToInt64(SimpStringPtr str, int64_t *value);
+int SimpleStringToInt64(SimpStringPtr str, int64 *value);
--- End diff --
Because the definition of `int64` is `long int` while the definition of `int64_t` is `long long int`. Besides, the caller of this function is a `HostnameVolumeInfo` object which is definited as a `int64`.
---
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] incubator-hawq issue #1036: HAWQ-1188. Fix "-Wtautological-constant-out-of-r...
Posted by xunzhang <gi...@git.apache.org>.
Github user xunzhang commented on the issue:
https://github.com/apache/incubator-hawq/pull/1036
cc @paul-guo- @huor
---
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] incubator-hawq pull request #1036: HAWQ-1188. Fix "-Wtautological-constant-o...
Posted by xunzhang <gi...@git.apache.org>.
Github user xunzhang commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/1036#discussion_r90875684
--- Diff: src/backend/cdb/cdbgang.c ---
@@ -444,7 +444,7 @@ disconnectAndDestroyGang(Gang *gp)
elog(DEBUG2, "Warning: disconnectAndDestroyGang called on an %s gang",
gp->active ? "active" : "allocated");
- if (gp->gang_id < 1 || gp->gang_id > 100000000 || gp->type > 10 || gp->size > 100000)
+ if (gp->gang_id < 1 || gp->gang_id > 100000000 || gp->type > GANGTYPE_PRIMARY_WRITER || gp->size > 100000)
--- End diff --
check >10 is meaningless. We can also add `GANGTYPE_MAX` then check with `gp->type >= GANGTYPE_MAX`. Also, we can check with `gp-type < GANGTYPE_MAX`. All of these are equivalent.
---
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] incubator-hawq pull request #1036: HAWQ-1188. Fix "-Wtautological-constant-o...
Posted by xunzhang <gi...@git.apache.org>.
Github user xunzhang closed the pull request at:
https://github.com/apache/incubator-hawq/pull/1036
---
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] incubator-hawq pull request #1036: HAWQ-1188. Fix "-Wtautological-constant-o...
Posted by xunzhang <gi...@git.apache.org>.
Github user xunzhang commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/1036#discussion_r91325169
--- Diff: src/backend/resourcemanager/include/utils/simplestring.h ---
@@ -63,7 +63,7 @@ bool SimpleStringEmpty(SimpStringPtr str);
int SimpleStringLocateChar(SimpStringPtr str, char target, int *location);
/* string to the other number values. */
int SimpleStringToInt32(SimpStringPtr str, int32_t *value);
-int SimpleStringToInt64(SimpStringPtr str, int64_t *value);
+int SimpleStringToInt64(SimpStringPtr str, int64 *value);
--- End diff --
Added.
---
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] incubator-hawq pull request #1036: HAWQ-1188. Fix "-Wtautological-constant-o...
Posted by paul-guo- <gi...@git.apache.org>.
Github user paul-guo- commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/1036#discussion_r90803332
--- Diff: src/backend/cdb/cdbsreh.c ---
@@ -347,11 +347,8 @@ void InsertIntoErrorTable(CdbSreh *cdbsreh)
}
/* file name */
- if(cdbsreh->filename)
- {
- values[errtable_filename - 1] = DirectFunctionCall1(textin, CStringGetDatum(cdbsreh->filename));
- nulls[errtable_filename - 1] = false;
- }
+ values[errtable_filename - 1] = DirectFunctionCall1(textin, CStringGetDatum(cdbsreh->filename));
+ nulls[errtable_filename - 1] = false;
--- End diff --
Why removing the if check?
---
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] incubator-hawq pull request #1036: HAWQ-1188. Fix "-Wtautological-constant-o...
Posted by paul-guo- <gi...@git.apache.org>.
Github user paul-guo- commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/1036#discussion_r90803660
--- Diff: src/backend/gp_libpq_fe/fe-exec.c ---
@@ -2668,7 +2668,7 @@ PQresultStatus(const PGresult *res)
char *
PQresStatus(ExecStatusType status)
{
- if ((int)status < 0 || status >= sizeof pgresStatus / sizeof pgresStatus[0])
+ if (status < PGRES_EMPTY_QUERY || (unsigned int) status >= sizeof pgresStatus / sizeof pgresStatus[0])
--- End diff --
Again, see my previous comment.
---
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] incubator-hawq pull request #1036: HAWQ-1188. Fix "-Wtautological-constant-o...
Posted by paul-guo- <gi...@git.apache.org>.
Github user paul-guo- commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/1036#discussion_r90803468
--- Diff: src/backend/gp_libpq_fe/fe-connect.c ---
@@ -436,7 +436,7 @@ PQconnectStartParams(const char **keywords,
{
conn->status = CONNECTION_BAD;
/* errorMessage is already set */
- return false;
+ return conn;
--- End diff --
The original code is absolutely buggy. However please double check the logic to see whether conn instead of others like NULL should be 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] incubator-hawq pull request #1036: HAWQ-1188. Fix "-Wtautological-constant-o...
Posted by paul-guo- <gi...@git.apache.org>.
Github user paul-guo- commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/1036#discussion_r90805114
--- Diff: src/interfaces/libpq/fe-exec.c ---
@@ -2686,7 +2686,7 @@ PQoidStatus(const PGresult *res)
size_t len;
- if (!res || !res->cmdStatus || strncmp(res->cmdStatus, "INSERT ", 7) != 0)
+ if (!res || strncmp(res->cmdStatus, "INSERT ", 7) != 0)
--- End diff --
See previous comments.
---
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] incubator-hawq pull request #1036: HAWQ-1188. Fix "-Wtautological-constant-o...
Posted by xunzhang <gi...@git.apache.org>.
Github user xunzhang commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/1036#discussion_r90877543
--- Diff: src/backend/gp_libpq_fe/fe-exec.c ---
@@ -2968,7 +2968,7 @@ PQoidStatus(const PGresult *res)
size_t len;
- if (!res || !res->cmdStatus || strncmp(res->cmdStatus, "INSERT ", 7) != 0)
+ if (!res || strncmp(res->cmdStatus, "INSERT ", 7) != 0)
--- End diff --
`res->cmdStatus` can not be NULL if `res` is not NULL. Please check out the definition of `PGresult`, it is a stack memory bundled with `res` variable.
---
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] incubator-hawq pull request #1036: HAWQ-1188. Fix "-Wtautological-constant-o...
Posted by xunzhang <gi...@git.apache.org>.
Github user xunzhang commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/1036#discussion_r90876168
--- Diff: src/backend/cdb/cdbsreh.c ---
@@ -347,11 +347,8 @@ void InsertIntoErrorTable(CdbSreh *cdbsreh)
}
/* file name */
- if(cdbsreh->filename)
- {
- values[errtable_filename - 1] = DirectFunctionCall1(textin, CStringGetDatum(cdbsreh->filename));
- nulls[errtable_filename - 1] = false;
- }
+ values[errtable_filename - 1] = DirectFunctionCall1(textin, CStringGetDatum(cdbsreh->filename));
+ nulls[errtable_filename - 1] = false;
--- End diff --
Because if `cdbsreh` is created, `cdbsreh->filename` should be also not NULL. In other words, this check is meaningless. Please check out the definition of `CdbSreh`.
---
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] incubator-hawq pull request #1036: HAWQ-1188. Fix "-Wtautological-constant-o...
Posted by paul-guo- <gi...@git.apache.org>.
Github user paul-guo- commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/1036#discussion_r90805107
--- Diff: src/interfaces/libpq/fe-exec.c ---
@@ -2386,7 +2386,7 @@ PQresultStatus(const PGresult *res)
char *
PQresStatus(ExecStatusType status)
{
- if (status < 0 || status >= sizeof pgresStatus / sizeof pgresStatus[0])
+ if (status < PGRES_EMPTY_QUERY || (unsigned int) status >= sizeof pgresStatus / sizeof pgresStatus[0])
--- End diff --
See previous comments.
---
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] incubator-hawq pull request #1036: HAWQ-1188. Fix "-Wtautological-constant-o...
Posted by xunzhang <gi...@git.apache.org>.
Github user xunzhang commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/1036#discussion_r91325465
--- Diff: src/backend/cdb/cdbgang.c ---
@@ -444,7 +444,7 @@ disconnectAndDestroyGang(Gang *gp)
elog(DEBUG2, "Warning: disconnectAndDestroyGang called on an %s gang",
gp->active ? "active" : "allocated");
- if (gp->gang_id < 1 || gp->gang_id > 100000000 || gp->type > 10 || gp->size > 100000)
+ if (gp->gang_id < 1 || gp->gang_id > 100000000 || gp->type > GANGTYPE_PRIMARY_WRITER || gp->size > 100000)
--- End diff --
added.
---
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] incubator-hawq pull request #1036: HAWQ-1188. Fix "-Wtautological-constant-o...
Posted by paul-guo- <gi...@git.apache.org>.
Github user paul-guo- commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/1036#discussion_r90803251
--- Diff: src/backend/cdb/cdbgang.c ---
@@ -444,7 +444,7 @@ disconnectAndDestroyGang(Gang *gp)
elog(DEBUG2, "Warning: disconnectAndDestroyGang called on an %s gang",
gp->active ? "active" : "allocated");
- if (gp->gang_id < 1 || gp->gang_id > 100000000 || gp->type > 10 || gp->size > 100000)
+ if (gp->gang_id < 1 || gp->gang_id > 100000000 || gp->type > GANGTYPE_PRIMARY_WRITER || gp->size > 100000)
--- End diff --
GANGTYPE_PRIMARY_WRITER seems to be not equal to 10. Are you sure GANGTYPE_PRIMARY_WRITER is the correct one? If you mean the max item of GangType, you might add a new entry in GangType, e.g
GANGTYPE_MAX or similar name.
By the way, why not check the < case?
---
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] incubator-hawq pull request #1036: HAWQ-1188. Fix "-Wtautological-constant-o...
Posted by ictmalili <gi...@git.apache.org>.
Github user ictmalili commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/1036#discussion_r90798944
--- Diff: src/backend/cdb/cdbgang.c ---
@@ -444,7 +444,7 @@ disconnectAndDestroyGang(Gang *gp)
elog(DEBUG2, "Warning: disconnectAndDestroyGang called on an %s gang",
gp->active ? "active" : "allocated");
- if (gp->gang_id < 1 || gp->gang_id > 100000000 || gp->type > 10 || gp->size > 100000)
+ if (gp->gang_id < 1 || gp->gang_id > 100000000 || gp->type > GANGTYPE_PRIMARY_WRITER || gp->size > 100000)
--- End diff --
Why extract this parameter out?
---
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] incubator-hawq pull request #1036: HAWQ-1188. Fix "-Wtautological-constant-o...
Posted by xunzhang <gi...@git.apache.org>.
Github user xunzhang commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/1036#discussion_r90877094
--- Diff: src/backend/gp_libpq_fe/fe-exec.c ---
@@ -2668,7 +2668,7 @@ PQresultStatus(const PGresult *res)
char *
PQresStatus(ExecStatusType status)
{
- if ((int)status < 0 || status >= sizeof pgresStatus / sizeof pgresStatus[0])
+ if (status < PGRES_EMPTY_QUERY || (unsigned int) status >= sizeof pgresStatus / sizeof pgresStatus[0])
--- End diff --
Here, `PGRES_EMPTY_QUERY` is a lower guard enum value, so `status < PGRES_EMPTY_QUERY` is ok.
---
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] incubator-hawq pull request #1036: HAWQ-1188. Fix "-Wtautological-constant-o...
Posted by paul-guo- <gi...@git.apache.org>.
Github user paul-guo- commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/1036#discussion_r90804941
--- Diff: src/backend/resourcemanager/include/utils/simplestring.h ---
@@ -63,7 +63,7 @@ bool SimpleStringEmpty(SimpStringPtr str);
int SimpleStringLocateChar(SimpStringPtr str, char target, int *location);
/* string to the other number values. */
int SimpleStringToInt32(SimpStringPtr str, int32_t *value);
-int SimpleStringToInt64(SimpStringPtr str, int64_t *value);
+int SimpleStringToInt64(SimpStringPtr str, int64 *value);
--- End diff --
Why change this?
---
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.
---