> On Oct 17, 2025, at 12:20, Tender Wang <tndrwang@gmail.com> wrote:
>
>
>
> Xuneng Zhou <xunengzhou@gmail.com> 于2025年10月17日周五 10:31写道：
> Hi Tender,
>
> On Thu, Oct 16, 2025 at 8:24 PM Tender Wang <tndrwang@gmail.com> wrote:
> >
> >
> >
> > Richard Guo <guofenglinux@gmail.com> 于2025年10月16日周四 17:53写道：
> >>
> >> On Thu, Oct 16, 2025 at 4:07 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> >> > On Mon, Sep 1, 2025 at 9:26 AM Chao Li <li.evan.chao@gmail.com> wrote:
> >> > > On Aug 30, 2025, at 14:09, Tender Wang <tndrwang@gmail.com> wrote:
> >> > > While debugging the HashJoin codes, I noticed below codes in ExecHashJoinImpl():
> >> > >
> >> > > elog(ERROR, "unrecognized hashjoin state: %d",
> >> > > (int) node->hj_JoinState);
> >> > >
> >> > > The type of hj_JoinState is already int, so the cast seems unnecessary.
> >> > > So I remove it in the attached patch.
> >>
> >> > > Yes, hj_JoinState is of type int, so the type cast to int is not needed. The change looks good to me.
> >>
> >> > LGTM.
> >>
> >> I think we can remove this cast, although it's so trivial that it
> >> doesn't seem to have any real impact. A similar cast also exists for
> >> mj_JoinState in ExecMergeJoin().
> >>
> >> (These values represent the state of the Join state machine for
> >> HashJoin and MergeJoin respectively, so I kind of wonder if it might
> >> be better to define them as enum rather than using int.)
> >
> >
> > Make sense.
> >
> > Please check the v2 patch.
>
> If we decide to converts HashJoin and MergeJoin state machine
> definitions from #define macros to enum types, we might need to keep
> the (int) casts from the elog() error messages:
>
> elog(ERROR, "unrecognized hashjoin state: %d", (int) node->hj_JoinState);
> elog(ERROR, "unrecognized mergejoin state: %d", (int) node->mj_JoinState);
>
> I searched the entire codebase and found that the accumStats() function is in pgbench.c,
> we have
> pg_fatal("unexpected error status: %d", estatus);
> and the estatus is enum type.
> So we can no need to use cast, too.
>
Passing enums directly to “%d” is acceptable, but it’s a good practice to do so explicitly.
As long as build doesn’t raise a warning, and Coverity doesn’t complain about it, it should be fine. Given the existing
usagethat Tender found, looks like Coverity won’t complain about it. So I think it’s fine.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
