[PATCH 0/1] MR3587: mshtml: Make skip_prefix static in OmNavigator_get_appVersion.
From: Alex Henrie <alexhenrie24(a)gmail.com> --- dlls/mshtml/omnavigator.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/mshtml/omnavigator.c b/dlls/mshtml/omnavigator.c index 646017aee9e..87333f3c239 100644 --- a/dlls/mshtml/omnavigator.c +++ b/dlls/mshtml/omnavigator.c @@ -1257,11 +1257,11 @@ static HRESULT get_user_agent(OmNavigator *navigator, WCHAR **user_agent) static HRESULT WINAPI OmNavigator_get_appVersion(IOmNavigator *iface, BSTR *p) { + static const unsigned skip_prefix = sizeof("Mozilla/") - 1; OmNavigator *This = impl_from_IOmNavigator(iface); WCHAR *user_agent; unsigned len; HRESULT hres; - const unsigned skip_prefix = 8; /* strlen("Mozilla/") */ TRACE("(%p)->(%p)\n", This, p); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3587
What's the point of this? It's just a simple constant, we never use its address, so it makes no sense for it to be static. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3587#note_42600
On Thu Aug 17 15:23:11 2023 +0000, Gabriel Ivăncescu wrote:
What's the point of this? It's just a simple constant, we never use its address, so it makes no sense for it to be static. The main advantage is that a static constant can be initialized from a sizeof, which tells the reader that there is little chance that the magic number was calculated incorrectly. A static constant is also more likely to be optimized out, although in the case of a single integer it doesn't matter much.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3587#note_42622
On Thu Aug 17 15:23:11 2023 +0000, Alex Henrie wrote:
The main advantage is that a static constant can be initialized from a sizeof, which tells the reader that there is little chance that the magic number was calculated incorrectly. A static constant is also more likely to be optimized out, although in the case of a single integer it doesn't matter much. Sorry, that was a poor explanation. A regular constant can also be initialized from a sizeof, so I could achieve the main goal of clarifying the code without changing the constant to be static.
Static constants do have another advantage besides optimization: They can be placed in read-only memory, so if someone editing this code did take a pointer to the constant and accidentally modify it, it would be obvious because the code would segfault. In my opinion that is a "nice to have" even though, like the optimization benefit, it doesn't matter very much in this particular case. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3587#note_42625
Static constants do have another advantage besides optimization: They can be placed in read-only memory, so if someone editing this code did take a pointer to the constant and accidentally modify it, it would be obvious because the code would segfault. In my opinion that is a "nice to have" even though, like the optimization benefit, it doesn't matter very much in this particular case.
Please let's avoid that kind of "optimization", it doesn't make any sense. I don't think there's anything to change it, but if you absolutely want to get rid of the magic number, just replace it by strlen(). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3587#note_42627
On Thu Aug 17 15:51:16 2023 +0000, Alexandre Julliard wrote:
Static constants do have another advantage besides optimization: They can be placed in read-only memory, so if someone editing this code did take a pointer to the constant and accidentally modify it, it would be obvious because the code would segfault. In my opinion that is a "nice to have" even though, like the optimization benefit, it doesn't matter very much in this particular case. Please let's avoid that kind of "optimization", it doesn't make any sense. I don't think there's anything to change it, but if you absolutely want to get rid of the magic number, just replace it by strlen(). In terms of optimization, as long as the address isn't taken, a const local variable will literally get replaced by the direct constant, as if using a #define macro or an enum, how is static better?
The compiler actually has to do *more* work to optimize out a static constant like this, since it has to actually remove the variable (because it's not addressed from anything via its address and not visible outside the compilation unit, so it can be removed, but still that's an extra effort it has to do). const local variables aren't even allocated as variables when optimizing. Even if they did, it's because they're caching some expensive calculation, which is actually good. static const makes sense when you want to have one pre-allocated instance of the variable, when you need to actually read it via its address. This is not the case here. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3587#note_42628
participants (4)
-
Alex Henrie -
Alex Henrie (＠alexhenrie)
-
Alexandre Julliard (＠julliard)
-
Gabriel Ivăncescu