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.