Re: Small fix on query_id_enabled

On Sat, 10 Feb 2024 10:19:15 +0900
Michael Paquier <michael@paquier.xyz> wrote:

> On Fri, Feb 09, 2024 at 04:37:23PM +0800, Julien Rouhaud wrote:
> > On Fri, Feb 09, 2024 at 03:38:23PM +0900, Yugo NAGATA wrote:
> >> 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.
> 
> Agreed.  A renaming would involve more pain than gain.  Improving the
> comments around how to all that would be good enough, my 2c.

Thank you both for your comments.

I agreed with not renaming it.

I attached a updated patch that adds comments noting to use IsQueryIdEnabled()
instead of accessing the variables directly.

Regards,
Yugo Nagata
-- 
Yugo NAGATA <nagata@sraoss.co.jp>