ut-match algorithm improvements. #1333
Conversation
alejandro-colomar
changed the title
IMO, it's more readable in this specific case, because it aligns the similar conditions. It also makes the other commits smaller. (In fact, it made me finally fully understand it, which I hadn't understood from the previous implementation.) |
"Smaller commits" should not be a reason for "worse code". } else if ( (match < UT_MATCH_BY_LINE)
&& (LOGIN_PROCESS == ut->ut_type) /* Be more picky when matching by 'ut_line' only */
&& is_my_tty(ut->ut_line))
{
match = UT_MATCH_BY_LINE;
ut_copy = *ut;
}vs } else if ( LOGIN_PROCESS == ut->ut_type /* Be more picky when matching by 'ut_line' only */
&& is_my_tty(ut->ut_line)) {
if (match < UT_MATCH_BY_LINE)
{
match = UT_MATCH_BY_LINE;
ut_copy = *ut;
}
}Simpler, shorter, better and more efficient. |
Karlson2k
left a comment
Karlson2k
left a comment
There was a problem hiding this comment.
Nested if() should be avoided.
I disagree. I like much better the latter (but I'd fix the braces). Both
|
|
Besides inconsistent styles, questionable readability improvements in some places, the code is fine. |
alejandro-colomar
force-pushed
the
ut
branch
from
9f55b18 to
791e81b
Compare
alejandro-colomar
force-pushed
the
ut
branch
2 times, most recently
from
a9f85f1 to
7c64a9b
Compare
alejandro-colomar
force-pushed
the
ut
branch
2 times, most recently
from
0a76cb4 to
7956194
Compare
alejandro-colomar
force-pushed
the
ut
branch
7 times, most recently
from
1b33b6b to
cf983b2
Compare
alejandro-colomar
force-pushed
the
ut
branch
2 times, most recently
from
051312b to
a090ce5
Compare
alejandro-colomar
force-pushed
the
ut
branch
2 times, most recently
from
8e4f43f to
b1a45f3
Compare
|
Besides mentioned sub-optimal processing, the current code actually improves utmp file processing. The utmp API is not thread-safe (not even multi-process safe) by its nature. This should work better. Any plans for a new release? |
Yup, we release around the solstices. The plans for the next release are here: |
Signed-off-by: Alejandro Colomar <[email protected]>
alejandro-colomar
force-pushed
the
ut
branch
from
b1a45f3 to
38a3661
Compare
This simplifies the implementation, allowing us to use _Generic(3) to replace _Static_assert(3), is_same_type(), and local variables. Local variables in macros can cause issues when nesting such macros. This also makes the code more readable, by having the type explicitly stated at call site. Signed-off-by: Alejandro Colomar <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
Have xxx_T() macros that only add type safety to the function they wrap. Have XXX() macros that do more magic; in this case, they deduce the comparison function that is appropriate for the type. This will allow using the xxx_T() macros in more complex cases, where the function can't be deduced just from the type. Signed-off-by: Alejandro Colomar <[email protected]>
This macro is like typeof(), but requires that the input is a type name.
This macro is useful because it parenthesizes the type name, which
allows one to create pointers to arbitrary types. The following code
works for simple types:
T x;
T *p;
For example, that would work fine for 'int':
int x;
int *p;
However, that wouldn't work for types such as 'int[3]':
int[3] x;
int[3] *p;
But if we do instead
typeas(T) x;
typeas(T) *p;
now we can use 'int[3]' just fine:
typeof((int[3]){}) x;
typeof((int[3]){}) *p;
To understand this, we create a compound literal of type int[3], with
default values (zero, zero, zero), then get it's type, which is
obviously int[3]. And then we use that to declare a variable x of that
type, and a pointer p, which has type 'int(*)[3]'.
This would also work with something simpler. One could use typeof(T)
directly:
typeof(T) x;
typeof(T) *p;
However, typeof() doesn't require that the input is a type; it accepts
arbitrary input. The following is valid C:
typeof(42) x;
For our macro MALLOC(), where we want the second argument to be a type
name, we want to require that the argument is a type name. For that, we
need to use typeas().
Reported-by: Alejandro Colomar <[email protected]>
Suggested-by: Thiago Adams <[email protected]>
Suggested-by: Martin Uecker <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
Casts are unsafe. Compound literals also have the ability of converting values, but they don't have the unwanted effects on safety --casts disable most useful diagnostics--. Compound literals are lvalues, which means their address can be taken, and they can also be assigned to. To avoid this, we force lvalue conversion through a statement expression. Signed-off-by: Alejandro Colomar <[email protected]>
This should be more readable. Signed-off-by: Alejandro Colomar <[email protected]>
The 'T' in the name notes that this API is a type-safe variant of the API it wraps. This makes the names more explicative. Signed-off-by: Alejandro Colomar <[email protected]>
The 'T' in the name notes that this API is a type-safe variant of the API it wraps. This makes the names more explicative. Signed-off-by: Alejandro Colomar <[email protected]>
The 'T' in the name notes that this API is a type-safe variant of the API it wraps. This makes the names more explicative. Signed-off-by: Alejandro Colomar <[email protected]>
The 'T' in the name notes that this API is a type-safe variant of the API it wraps. This makes the names more explicative. Signed-off-by: Alejandro Colomar <[email protected]>
The 'T' in the name notes that this API is a type-safe variant of the API it wraps. This makes the names more explicative. Signed-off-by: Alejandro Colomar <[email protected]>
The 'T' in the name notes that this API is a type-safe variant of the API it wraps. This makes the names more explicative. Signed-off-by: Alejandro Colomar <[email protected]>
The 'T' in the name notes that this API is a type-safe variant of the API it wraps. This makes the names more explicative. Signed-off-by: Alejandro Colomar <[email protected]>
We don't use this value. This silences a diagnostic about the unused return value. Signed-off-by: Alejandro Colomar <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
This function already returned NULL on some errors. It didn't make any sense to exit(3) on allocation failure. Instead, just return NULL. Signed-off-by: Alejandro Colomar <[email protected]>
Cc: "Evgeny Grin (Karlson2k)" <[email protected]> Signed-off-by: Alejandro Colomar <[email protected]>
This aligns similar code, enhancing readability of the algorithm. Signed-off-by: Alejandro Colomar <[email protected]>
This tells in a more readable way what kind of match we have. I'll use this enum for simplifying other stuff in the following commit. Signed-off-by: Alejandro Colomar <[email protected]>
As Evgeny reported, malloc(3) is overkill here. He suggested using other methods, which are less than ideal, but it's true that it's overkill, and with the stack we can get something readable and more efficient. Reported-by: Evgeny Grin (Karlson2k) <[email protected]> Signed-off-by: Alejandro Colomar <[email protected]>
This removes the reuse of 'ut', with which I wasn't entirely happy. It also reduces the scope between setutxent()/endutxent(). Signed-off-by: Alejandro Colomar <[email protected]>
3 participants
I've optimized the algorithm in several ways (both performance and readability):
Reported-by: @Karlson2k
Queued after #1296 .
Revisions:
v2
v2b
v2c
v2d
v3
v3b
v3c
v4
v4b
v5
v5b
v5c
v5d
v5e
v5f
v5g
v5h
v5i
v5j
v5k
v5l
v5m
v5n