On 01.09.25 05:32, Chao Li wrote:
> 1. For 0001, I think “#ifdef YY_TYPEDEF_YY_SCANNER_T” is not needed, but
> I see you have removed that in 0002, so this is not a comment now.
Yes, this is just so that the code after applying patch 0001 still
works, even if some code is later removed.
> 2. For 0002, looks like som duplicate code for definitions of
>
> Union YYSTYPE;
> #define YYLTYPE int
> Typedef void *yyscan_t;
>
> Can we put them into a separate header file say src/include/yy_types.h?
Each scanner/parser might have its own idea of what type it wants to
use, so this might not work. It's better not to get into the
complications of this for this patch set.
> 3. For 0002, this is not your change, I am just pointing it out:
> “#define YYSTYPE char *”, where “YYSTYPE” is defined as “union”
> everywhere else. Can we rename it to something like “YYSTYPE_CHARPTR”
> here to avoid confusion? I tried to rename it and build still passed.
The name YYSTYPE is required by bison. I'm not sure how it still works
without it. But anyway, see above, it's probably better not to crack
open the bison/flex mess again here.
> 4. For 0004
>
> diff --git a/src/include/commands/explain_dr.h b/src/include/commands/
> explain_dr.h
> index 55da63d66bd..ce424aa2a55 100644
> --- a/src/include/commands/explain_dr.h
> +++ b/src/include/commands/explain_dr.h
> @@ -16,7 +16,7 @@
> #include "executor/instrument.h"
> #include "tcop/dest.h"
>
> -struct ExplainState; /* avoid including explain.h here */
> +typedef struct ExplainState ExplainState; /* avoid including
> explain.h here */
>
> “Struct ExplainState” is defined in explain_state.h, so the comment here
> should be “avoid including explain_state.h here”.
>
> Same thing applies to explain_format.h and fdwapi.h.
Yes, good catch. The original patch that introduced these comments
actually did replace an include of explain.h, but arguably even that was
already too broad. So I agree we should fix these comments.
> 5. For 0005
>
> diff --git a/src/include/optimizer/optimizer.h b/src/include/optimizer/
> optimizer.h
> index 03b214755c2..d490ef3b506 100644
> --- a/src/include/optimizer/optimizer.h
> +++ b/src/include/optimizer/optimizer.h
> @@ -35,9 +35,9 @@ typedef struct IndexOptInfo IndexOptInfo;
> typedef struct SpecialJoinInfo SpecialJoinInfo;
>
> /* It also seems best not to include plannodes.h, params.h, or htup.h
> here */
> -struct PlannedStmt;
> -struct ParamListInfoData;
> -struct HeapTupleData;
> +typedef struct PlannedStmt PlannedStmt;
> +typedef struct ParamListInfoData ParamListInfoData;
> +typedef struct HeapTupleData *HeapTuple;
>
> Why don’t define ParamListInfo in the same way as HeapTuple like
> “typedef structure ParamListInfoData *ParamListInfo”. I tried that in my
> local and build still passed.
Yes, that seems more correct.
Thanks. New patch set attached.
