Details
-
Task
-
Status: Closed (View Workflow)
-
Minor
-
Resolution: Fixed
Description
Some class should store an old value and change it's value to a new/tmp value in ctor, and restore an old value in dtor.
Attachments
Issue Links
- relates to
-
-
- Closed
-
Activity
| Field | Original Value | New Value |
|---|---|---|
| Link |
This issue relates to |
| Component/s | Server [ 13907 ] |
| Fix Version/s | 10.2 [ 14601 ] | |
| Fix Version/s | 10.3 [ 22126 ] | |
| Fix Version/s | 10.4 [ 22408 ] | |
| Fix Version/s | 10.5 [ 23123 ] |
| Labels | refactoring |
| Status | Open [ 1 ] | In Progress [ 3 ] |
| Assignee | Eugene Kosov [ kevg ] | Sergei Golubchik [ serg ] |
| Status | In Progress [ 3 ] | In Review [ 10002 ] |
| Assignee | Sergei Golubchik [ serg ] | Eugene Kosov [ kevg ] |
| Status | In Review [ 10002 ] | Stalled [ 10000 ] |
| Summary | implement a generic way to change a value of a variable in scope | implement a generic way to change a value of a variable in a scope |
| Fix Version/s | 10.7 [ 24805 ] | |
| Fix Version/s | 10.2 [ 14601 ] | |
| Fix Version/s | 10.3 [ 22126 ] | |
| Fix Version/s | 10.4 [ 22408 ] | |
| Fix Version/s | 10.5 [ 23123 ] |
| Assignee | Eugene Kosov [ kevg ] | Sergei Golubchik [ serg ] |
| Status | Stalled [ 10000 ] | In Review [ 10002 ] |
| Fix Version/s | 10.8 [ 26121 ] | |
| Fix Version/s | 10.7 [ 24805 ] |
| Workflow | MariaDB v3 [ 108167 ] | MariaDB v4 [ 131776 ] |
| Assignee | Sergei Golubchik [ serg ] | Eugene Kosov [ kevg ] |
| Status | In Review [ 10002 ] | Stalled [ 10000 ] |
| Assignee | Eugene Kosov [ kevg ] | Sergei Golubchik [ serg ] |
| Status | Stalled [ 10000 ] | In Review [ 10002 ] |
| Fix Version/s | 10.10 [ 27530 ] | |
| Fix Version/s | 10.8 [ 26121 ] |
| Fix Version/s | 10.11 [ 27614 ] | |
| Fix Version/s | 10.10 [ 27530 ] |
| Description | Some class should store an old value and change it's value to a new/tmp value in ctor, and restore an old value in dtor. |
Some class should store an old value and change it's value to a new/tmp value in ctor, and restore an old value in dtor.
PLEASE DO NOT PUSH! This goes against the codeing standard in MariaDB! - Do not use C++ construct that hides what is happening! - Makes code harder to read and understand for C programmers! Do not use 'auto' in MariaDB! This is a bad programming structure that makes the code harder to read and understand |
| Priority | Major [ 3 ] | Minor [ 4 ] |
| Description |
Some class should store an old value and change it's value to a new/tmp value in ctor, and restore an old value in dtor.
PLEASE DO NOT PUSH! This goes against the codeing standard in MariaDB! - Do not use C++ construct that hides what is happening! - Makes code harder to read and understand for C programmers! Do not use 'auto' in MariaDB! This is a bad programming structure that makes the code harder to read and understand |
Some class should store an old value and change it's value to a new/tmp value in ctor, and restore an old value in dtor. |
| Assignee | Sergei Golubchik [ serg ] | Aleksey Midenkov [ midenok ] |
| Status | In Review [ 10002 ] | Stalled [ 10000 ] |
| Status | Stalled [ 10000 ] | In Progress [ 3 ] |
| Assignee | Aleksey Midenkov [ midenok ] | Nikita Malyavin [ nikitamalyavin ] |
| Status | In Progress [ 3 ] | In Review [ 10002 ] |
I think that we can have it in a code base, however it should be used with some causion: it's better to control the behavior with function arguments, rather than semi-global variables. Historically, THD::sql_mode and THD::abort_on_warning are the two exceptions.
midenok I think that SCOPE_MASK_SET and SCOPE_MASK_CLEAR names will do better than SCOPE_SET and SCOPE_CLEAR. Or, maybe, SCOPE_MASK_ADD and SCOPE_MASK_REMOVE.
Also I think that CONCAT and ANONYMOUS_VARIABLE may find a wider use, so I'd suggest to put them in some more generic header: it can be my_global.h, or another new header, like macro_utilities.h
With some better namings for SCOPE_SET/CLEAR, commits d5710a4423 and 86bc9aa0aa will be ok to push.
| Status | In Review [ 10002 ] | Stalled [ 10000 ] |
| Assignee | Nikita Malyavin [ nikitamalyavin ] | Aleksey Midenkov [ midenok ] |
| Fix Version/s | 10.5.28 [ 29952 ] | |
| Fix Version/s | 10.11 [ 27614 ] | |
| Resolution | Fixed [ 1 ] | |
| Status | Stalled [ 10000 ] | Closed [ 6 ] |
How did this get pushed into 10.5 just a week ago or so?
This is a code refactoring, it it not appropriate for any GA release, let alone earliest supported GA 10.5.
Where is the review of this patch, who approved it for push to 10.5?
Unless there is an explanation for why this should go in 10.5, it needs to be reverted before next release.
knielsen this was approved by serg. It goes to 10.5 because MDEV-22438 is in 10.5 and this is second part of that task. As long as this refactoring is harmless we want to keep it in 10.5 to avoid merge conflicts unless you have any counterarguments that this can inflict any harm.
midenok Ok, but I did not see any explanation for this in either this issue or in the commit comments, nor do I see serg's review anywhere. Refactoring in general is very much not appropriate for GA releases, so when there is a particular reason to do it anyway it is important that this reason is explained somewhere.
Wait, it must be some mistake. I wasn't a reviewer, Nikita was.
If I'd been a reviewer, though, I'd say it could go into 10.5, being, basically, hardly worse then renaming a variable.
Correct, I was a reviewer.
This is a code refactoring, it it not appropriate for any GA release
I know no such policy for Foundation.
knielsen Got it. We were discussing this in private chat. Next time I'll notify explicitly permission granted.
P.S. And yes, Nikita was the reviewer, but first I got an approval from Sergei.
As a followup note, this code works with gcc 4.8 and it is tested not only by tests, but by bootstrap as well.
https://github.com/MariaDB/server/commits/bb-10.7-MDEV-22441-scoped-variable