m_option: don't overlap UPDATE and M_OPT constant values #15457
Conversation
guidocella
force-pushed
the
option-constants
branch
from
632096c to
5744771
Compare
guidocella
force-pushed
the
option-constants
branch
2 times, most recently
from
049dcf7 to
0939db8
Compare
guidocella
force-pushed
the
option-constants
branch
from
0939db8 to
3f76607
Compare
guidocella
force-pushed
the
option-constants
branch
2 times, most recently
from
1ff9a1f to
0fb6d24
Compare
options/m_option.h
Outdated
|
|
||
| // Like M_OPT_TYPE_OPTIONAL_PARAM. | ||
| #define M_OPT_OPTIONAL_PARAM (1 << 30) | ||
| #define M_OPT_OPTIONAL_PARAM (UINT64_C(1) << 63) |
There was a problem hiding this comment.
Could we start from 63 and count down? So we can add new flags here instead of before NAN one.
So NAN would be 64, ALLOW_NO, would be 62 and so on.
There was a problem hiding this comment.
And after last flag we can add static_assert(UPDATE_OPT_LAST < M_OPT_OPTIONAL_PARAM), so it is easily visible requirement.
guidocella
force-pushed
the
option-constants
branch
from
0fb6d24 to
4c74951
Compare
|
Download the artifacts for this pull request: |
guidocella
force-pushed
the
option-constants
branch
2 times, most recently
from
64aed6c to
78a995e
Compare
guidocella
force-pushed
the
option-constants
branch
from
78a995e to
1c679ef
Compare
options/m_option.h
Outdated
| // type time: string "no" maps to MP_NOPTS_VALUE (if unset, NOPTS is | ||
| // rejected) and parsing: "--no-opt" is parsed as "--opt=no" | ||
| M_OPT_DEFAULT_NAN = (1 << 5), | ||
| M_OPT_ALLOW_NO = (1 << 6), |
There was a problem hiding this comment.
Forgot the comment for M_OPT_ALLOW_NO.
options/m_option.h
Outdated
| UPDATE_IMGPAR = (1 << 14), // video image params overrides | ||
| UPDATE_INPUT = (1 << 15), // mostly --input-* options | ||
| UPDATE_AUDIO = (1 << 16), // --audio-channels etc. | ||
| UPDATE_PRIORITY = (1 << 17), // --priority (1Windows-only) |
| #define UPDATE_OPT_LAST (1 << 26) | ||
|
|
||
| // All bits between _FIRST and _LAST (inclusive) | ||
| enum option_flags { |
There was a problem hiding this comment.
I think this shouldn't use enum here. I consider a value of enum type can only be one of the defined exact value instead of some OR'd values.
If you want to group flags you can add a comment at the beginning of this section like the "These flags are used to describe special parser capabilities or behavior." for the next section below.
There was a problem hiding this comment.
It was kasper's preference to use enum. I think it's more readable without all the #define and don't see a problem OR'ing constants as long as the OR'd variables are int64_t instead of the enum type.
There was a problem hiding this comment.
For me it is more easily recognizable, what flags are grouped together, but I approved previous version, as it was looking nice too. Frankly I would leave the enum conversion to separate commit, if not PR, to not mix it with real fixes that are being made in this changes.
There was a problem hiding this comment.
as the OR'd variables are int64_t instead of the enum type.
ok but that sounds easy to get wrong, no?
There was a problem hiding this comment.
It's not like it doesn't work if the variable is of the enum type, it just makes more sense to use int if the value is not one of the enum constants. Also even this example in Microsoft's docs does it and even assigns the OR'd constants to Days variables: https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/builtin-types/enum#enumeration-types-as-bit-flags
There was a problem hiding this comment.
I wouldn't take MS documentation as the benchmark of correctness. There are many code examples from them that don't even work.
In fact, C++ enum classes are strongly typed and won't allow such usage, just like enum in most non-C programming languages.
guidocella
force-pushed
the
option-constants
branch
from
1c679ef to
a6308fc
Compare
|
Needs rebase. |
Make these more readable by grouping them visually and removing all the #defines. This makes it clear that the M_OPT_TYPE constants are unrelated to these.
So that they will already work after more UPDATE flags are added. change_flags in m_config_core.h was already uint64_t. Co-authored-by: Kacper Michajłow <kasper93@gmail.com>
guidocella
force-pushed
the
option-constants
branch
from
a6308fc to
e4cc7f2
Compare
options/m_option.h
Outdated
| UPDATE_CLIPBOARD = (1 << 17), // reinit the clipboard | ||
| UPDATE_OPT_LAST = (1 << 17), | ||
|
|
||
| M_OPT_NOCFG = (1 << 30), // The option is forbidden in config files. |
There was a problem hiding this comment.
Now that the type has been changed to 64 bits, those should be moved up.
Add UINT64_C() and make them end from 61. This will allow adding more UPDATE flags without changing all option_flags.
|
I think we can wait with this PR until next time we add new flag. |
UPDATE_CLIPBOARD currently has the same value as M_OPT_DEFAULT_NAN, so increment all constants after UPDATE_OPT_LAST. But increment them by 5 so we can add a few more UPDATE flags without either forgetting to increase them like in e1d30c4, or having to increase them each time like in a5937ac.