Thread: static assert cleanup
A number of static assertions could be moved to better places. We first added StaticAssertStmt() in 2012, which required all static assertions to be inside function bodies. We then added StaticAssertDecl() in 2020, which enabled static assertions on file level. We have a number of calls that were stuck in not-really-related functions for this historical reason. This patch set cleans that up. 0001-Update-static-assert-usage-comment.patch This updates the usage information in c.h to be more current and precise. 0002-Move-array-size-related-static-assertions.patch This moves some obviously poorly placed ones. 0003-Move-some-static-assertions-to-better-places.patch This moves some that I thought were suboptimally placed but it could be debated or refined. 0004-Use-StaticAssertDecl-where-possible.patch This just changes some StaticAssertStmt() to StaticAssertDecl() where appropriate. It's optional.
Attachment
On Fri, Dec 9, 2022 at 2:47 PM Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
>
> 0003-Move-some-static-assertions-to-better-places.patch
>
> This moves some that I thought were suboptimally placed but it could be
> debated or refined.
+ * We really want ItemPointerData to be exactly 6 bytes. This is rather a
+ * random place to check, but there is no better place.
Since the assert is no longer in a random function body, it seems we can remove the second sentence.
--
John Naylor
EDB: http://www.enterprisedb.com
On Fri, Dec 9, 2022 at 6:47 PM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:
>
> A number of static assertions could be moved to better places.
>
> We first added StaticAssertStmt() in 2012, which required all static
> assertions to be inside function bodies. We then added
> StaticAssertDecl() in 2020, which enabled static assertions on file
> level. We have a number of calls that were stuck in not-really-related
> functions for this historical reason. This patch set cleans that up.
>
> 0001-Update-static-assert-usage-comment.patch
>
> This updates the usage information in c.h to be more current and precise.
>
> 0002-Move-array-size-related-static-assertions.patch
>
> This moves some obviously poorly placed ones.
>
> 0003-Move-some-static-assertions-to-better-places.patch
>
> This moves some that I thought were suboptimally placed but it could be
> debated or refined.
>
> 0004-Use-StaticAssertDecl-where-possible.patch
>
> This just changes some StaticAssertStmt() to StaticAssertDecl() where
> appropriate. It's optional.
Patch 0002
diff --git a/src/backend/utils/cache/syscache.c
b/src/backend/utils/cache/syscache.c
index eec644ec84..bb3dd6f4d2 100644
--- a/src/backend/utils/cache/syscache.c
+++ b/src/backend/utils/cache/syscache.c
@@ -1040,6 +1040,9 @@ static const struct cachedesc cacheinfo[] = {
}
};
+StaticAssertDecl(SysCacheSize == (int) lengthof(cacheinfo),
+ "SysCacheSize does not match syscache.c's array");
+
static CatCache *SysCache[SysCacheSize];
In almost every example I found of StaticAssertXXX, the lengthof(arr)
part came first in the condition. Since you are modifying this anyway,
should this one also be reversed for consistency?
======
Patch 0004
diff --git a/src/backend/executor/execExprInterp.c
b/src/backend/executor/execExprInterp.c
index 1dab2787b7..ec26ae506f 100644
--- a/src/backend/executor/execExprInterp.c
+++ b/src/backend/executor/execExprInterp.c
@@ -496,7 +496,7 @@ ExecInterpExpr(ExprState *state, ExprContext
*econtext, bool *isnull)
&&CASE_EEOP_LAST
};
- StaticAssertStmt(EEOP_LAST + 1 == lengthof(dispatch_table),
+ StaticAssertDecl(EEOP_LAST + 1 == lengthof(dispatch_table),
"dispatch_table out of whack with ExprEvalOp");
Ditto the previous comment.
------
Kind Regards,
Peter Smith.
Fujitsu Australia
On 11.12.22 23:18, Peter Smith wrote: > +StaticAssertDecl(SysCacheSize == (int) lengthof(cacheinfo), > + "SysCacheSize does not match syscache.c's array"); > + > static CatCache *SysCache[SysCacheSize]; > > In almost every example I found of StaticAssertXXX, the lengthof(arr) > part came first in the condition. Since you are modifying this anyway, > should this one also be reversed for consistency? Makes sense. I have pushed this separately.
On 09.12.22 11:01, John Naylor wrote: > > On Fri, Dec 9, 2022 at 2:47 PM Peter Eisentraut > <peter.eisentraut@enterprisedb.com > <mailto:peter.eisentraut@enterprisedb.com>> wrote: > > > > 0003-Move-some-static-assertions-to-better-places.patch > > > > This moves some that I thought were suboptimally placed but it could be > > debated or refined. > > + * We really want ItemPointerData to be exactly 6 bytes. This is rather a > + * random place to check, but there is no better place. > > Since the assert is no longer in a random function body, it seems we can > remove the second sentence. Committed with the discussed adjustments.