Hi,
On Fri, Feb 09, 2024 at 03:38:23PM +0900, Yugo NAGATA wrote:
>
> I found the comment on query_id_enabled looks inaccurate because this is
> never set to true when compute_query_id is ON.
>
> /* True when compute_query_id is ON, or AUTO and a module requests them */
> bool query_id_enabled = false;
>
> Should we fix this as following (just fixing the place of a comma) ?
>
> /* True when compute_query_id is ON or AUTO, and a module requests them */
Agreed.
> Also, I think the name is a bit confusing for the same reason, that is,
> query_id_enabled may be false even when query id is computed in fact.
>
> Actually, this does not matter because we use IsQueryIdEnabled to check
> if query id is enabled, instead of referring to a global variable
> (query_id_enabled or compute_query_id). But, just for making a code a bit
> more readable, how about renaming this to query_id_required which seems to
> stand for the meaning more correctly?
-1 for renaming to avoid breaking extensions that might access it. We should
simply document for compute_query_id and query_id_enabled declaration that one
should instead use IsQueryIdEnabled() if they're interested in whether the core
queryid are computed or not.