Thread: Small fix on query_id_enabled
Hi, 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 */ 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? I attached a patch for above fixes. Although renaming might not be acceptable since changing global variables may affect third party tools, I think the comment should be fixed at least. IMHO, it seems better to make this variable static not to be accessed directly. However, I left it as is because this is used in a static inline function. Regards, Yugo Nagata -- Yugo NAGATA <nagata@sraoss.co.jp>
Attachment
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.
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. -- Michael
Attachment
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>
Attachment
On Tue, Feb 13, 2024 at 01:13:43AM +0900, Yugo NAGATA wrote: > I attached an updated patch that adds comments noting to use IsQueryIdEnabled() > instead of accessing the variables directly. Sounds good to me, thanks. -- Michael
Attachment
On Tue, Feb 13, 2024 at 05:28:32PM +0900, Michael Paquier wrote: > On Tue, Feb 13, 2024 at 01:13:43AM +0900, Yugo NAGATA wrote: > > I attached an updated patch that adds comments noting to use IsQueryIdEnabled() > > instead of accessing the variables directly. > > Sounds good to me, thanks. +1!
On Tue, Feb 13, 2024 at 11:23:47PM +0800, Julien Rouhaud wrote: > +1! Okay, applied as-is, then. -- Michael
Attachment
On Wed, 14 Feb 2024 07:21:54 +0900 Michael Paquier <michael@paquier.xyz> wrote: > On Tue, Feb 13, 2024 at 11:23:47PM +0800, Julien Rouhaud wrote: > > +1! > > Okay, applied as-is, then. Thank you! Regards, Yugo Nagata > -- > Michael -- Yugo NAGATA <nagata@sraoss.co.jp>