Skip to content

Commit

Permalink
[#25810] YSQL: Lint whitespace for comments with non-word characters
Browse files Browse the repository at this point in the history
Summary:
`fn(/*example=*/false)` did not raise a lint error, but `fn(/*example*/false)` did.
This is because the regex did not account for other characters inside the comment.

Update the regex, avoid a few PG cases / files that don't follow this pattern, and make some small changes in YB-owned lines to conform to this rule.
Jira: DB-15108

Test Plan:
Tested on the following cases.
```
/* Good examples */
/*********/
/***** title *****/
/* ---------------------- TS Template commands -----------------------*/
/*-------------*/
/* example */
/* good example */
/*- translator: both %s are strings of the form "option = value" */
/*#define TRACE_VISIBILITYMAP */

/* Bad Examples */

/*=============*/
/* no whitespace after*/
/*no whitespace before */
/*no whitespace before or after*/
/* noWhiteSpaceAfter*/
/*noWhiteSpaceBefore */
/*noWhiteSpaceBeforeOrAfter*/
/* no whitespace after=*/
/*no whitespace before= */
/*no whitespace before or after=*/
/* noWhiteSpaceAfter=*/
/*noWhiteSpaceBefore= */
/*noWhiteSpaceBeforeOrAfter=*/
/* hello*/, myvar /*helloagain */

blah(/*example=*/false)
blah(/*example*/false)

/*a*/
/*ab*/
/*abc*/
```

```lang=sh, name=test-linter.sh
arc patch D41598

git checkout HEAD~
find src/postgres -type f \( -name "*.c" -o -name "*.h" \) -print0 | xargs -0 arc lint > before.txt
git checkout -
find src/postgres -type f \( -name "*.c" -o -name "*.h" \) -print0 | xargs -0 arc lint > after.txt

diff <(grep "comment_spacing" before.txt -A 7 -B 1) <(grep "comment_spacing" after.txt -A 7 -B 1)

NUM_COMMENT_SPACING_ERRORS_BEFORE=$(grep "comment_spacing" before.txt | wc -l)
NUM_COMMENT_SPACING_ERRORS_AFTER=$(grep "comment_spacing" after.txt | wc -l)

if (( NUM_COMMENT_SPACING_ERRORS_BEFORE - 1 != NUM_COMMENT_SPACING_ERRORS_AFTER )); then
  echo "Expected one less comment_spacing error after D41598"
  echo "Before:" $NUM_COMMENT_SPACING_ERRORS_BEFORE
  echo "After:" $NUM_COMMENT_SPACING_ERRORS_AFTER
  exit 1
fi
```

`test-linter.sh` shows the difference:
```
<
<    Error  (S&RX) bad_parameter_comment_spacing
<     Spacing should be like /* param */
<
<            12309 #  endif
<            12310 #endif
<            12311 #ifndef NOOP
<     >>>    12312 #  define NOOP                           /*EMPTY*/(void)0#  define NOOP                           /*EMPTY*/(void)0
<            12313 #endif
```

The difference shows just one line that used to be a lint problem, and is not any more.

No bad comment spacing complaints:
```lang=sh
grep "comment_spacing" after.txt -A 7 -B 1
```

Reviewers: jason

Reviewed By: jason

Subscribers: jason, yql

Differential Revision: https://phorge.dev.yugabyte.com/D41598
  • Loading branch information
timothy-e committed Feb 25, 2025
1 parent 5329249 commit 0385e06
Show file tree
Hide file tree
Showing 6 changed files with 29 additions and 8 deletions.
25 changes: 23 additions & 2 deletions src/lint/postgres.sh
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,30 @@ if ! [[ "$1" == src/postgres/src/backend/snowball/libstemmer/* ||
grep -nvE '^('$'\t''* {0,3}\S|$)' "$1" \
| sed 's/^/error:leading_whitespace:Remove leading whitespace:/'
fi
grep -nE '/\*(\w+|\s\w+|\w+\s)\*/' "$1" \
| sed 's,^,error:bad_parameter_comment_spacing:'\

# there are three cases to catch:
# 1. /*no whitespace before/after*/
# 2. /*no whitespace before */
# 3. /* no whitespace after*/
# We search for strings that do not start or end with a whitespace: \S+(.*\S+)?
# leading / trailing whitespace is not caught by that regex, so we can check it
# explicitly.
#
# second grep removes lines that have continuous ---, *** characters
# third grep removes lines that have /*#define or /*-, because PG uses those sometimes
if ! [[ "$1" == src/postgres/src/backend/snowball/libstemmer/* ||
"$1" == src/postgres/src/backend/utils/activity/pgstat.c ||
"$1" == src/postgres/src/interfaces/ecpg/test/expected/* ||
"$1" == src/postgres/contrib/pgcrypto/px-crypt.h ||
"$1" == src/postgres/src/include/tsearch/dicts/regis.h ||
"$1" == src/postgres/src/pl/plperl/ppport.h ]]; then
grep -nE '/\*(\S+(.*\S+)?|\s\S+(.*\S+)?|\S+(.*\S+)?\s)\*/' "$1" \
| grep -vE '[\*\-]{3}' \
| grep -vE '/\*(#define|\-)' \
| sed 's,^,error:bad_comment_spacing:'\
'Spacing should be like /* param */:,'
fi

if ! [[ "$1" == src/postgres/contrib/ltree/* ||
"$1" == src/postgres/src/backend/snowball/libstemmer/* ||
"$1" == src/postgres/src/backend/utils/adt/tsquery.c ||
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ YbClearEnumLabelMap(void)
ctl.keysize = YB_ENUM_LABEL_ASSIGNMENT_MAP_KEY_SIZE;
ctl.entrysize = sizeof(YbEnumLabelAssignmentMapEntry);
yb_enum_label_assignment_map = hash_create("YB enum label map",
/*initial size*/ 20, &ctl,
/* initial size */ 20, &ctl,
HASH_ELEM | HASH_STRINGS);
}

Expand Down Expand Up @@ -137,7 +137,7 @@ YbClearSequenceOidMap(void)
ctl.keysize = YB_SEQUENCE_OID_ASSIGNMENT_MAP_KEY_SIZE;
ctl.entrysize = sizeof(YbSequenceOidAssignmentMapEntry);
yb_sequence_oid_assignment_map = hash_create("YB sequence OIDs map",
/*initial size*/ 20, &ctl,
/* initial size */ 20, &ctl,
HASH_ELEM | HASH_STRINGS);
}

Expand Down
2 changes: 1 addition & 1 deletion src/postgres/src/backend/utils/misc/pg_yb_utils.c
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ YbResetCatalogCacheVersion()
yb_pgstat_set_catalog_version(yb_catalog_cache_version);
}

/** These values are lazily initialized based on corresponding environment variables. */
/* These values are lazily initialized based on corresponding environment variables. */
int ybc_pg_double_write = -1;
int ybc_disable_pg_locking = -1;

Expand Down
2 changes: 1 addition & 1 deletion src/postgres/src/include/nodes/execnodes.h
Original file line number Diff line number Diff line change
Expand Up @@ -2217,7 +2217,7 @@ typedef enum YbNLBatchStatus
BNL_FLUSHING
} YbNLBatchStatus;

/* Struct to contain tuple and its matching info in a hash bucket*/
/* Struct to contain tuple and its matching info in a hash bucket */
typedef struct YbBucketTupleInfo
{
MinimalTuple tuple;
Expand Down
2 changes: 1 addition & 1 deletion src/postgres/src/include/optimizer/ybplan.h
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ extern void yb_extract_pushdown_clauses(List *restrictinfo_list,
List **rel_colrefs, List **idx_remote_quals,
List **idx_colrefs);

/* YbSkippableEntities helper functions*/
/* YbSkippableEntities helper functions */
extern YbSkippableEntities *YbInitSkippableEntities(List *no_update_index_list);
extern void YbCopySkippableEntities(YbSkippableEntities *dst,
const YbSkippableEntities *src);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ SPI_TextArrayGetElement(HeapTuple spi_tuple, int column_id, int element_index)

get_typlenbyvalalign(element_type, &typlen, &typbyval, &typalign);
deconstruct_array(array, element_type, typlen, typbyval, typalign,
&elements, /*elog on NULL values*/ NULL,
&elements, /* elog on NULL values */ NULL,
&num_elements);

if (element_index >= 0 && element_index < num_elements)
Expand Down

0 comments on commit 0385e06

Please sign in to comment.