-
Notifications
You must be signed in to change notification settings - Fork 165
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
C cleanups #107
C cleanups #107
Conversation
They were missed with previous cleanups since they use '#if 0' (multiple spaces before the 0), and I did a simple git grep 'if 0'
"static" should be first -Wold-style-declaration
Tectonic does not stop to ask user for input, thus we remove the code potentially asking the user for a password.
do_encryption is initialized to false, it can only change on the else case when calling dvi_scan_specials.
Standard C libraries implement the free function that already checks if NULL is provided. In such case, free does nothing. For example, see ISO/IEC 9899: "7.20.3.2 The free function" Done using the following coccinelle semantic patch: @@ expression p; @@ - if (p) free(p);
Simplify a common pattern where one free a pointer, then assigns it NULL, with a single call. Done using the following coccinelle semantic patch: @@ expression p; @@ - free(p); - p = NULL; + p = mfree(p);
NULL is the value associated to a non-initialized pointer (it does not refer to a valid object). It improves readability to uses the NULL macro instead of a 0. Done using the following coccinelle patch: http://coccinelle.lip6.fr/rules/badzero.html
In C, a cast from void * to any other pointer is made implicitly. Done using the following coccinelle patch: @@ type T; @@ - (T *) (\(malloc\|xmalloc\|xcalloc\|calloc\)(...))
We use xmalloc/xcalloc to abort in case an OOM occurs.
It improves readability of the code. Done with the following coccinelle semantic patchs: @@ expression a, b; @@ - strcmp(a, b) == 0 + streq_ptr(a, b) @@ expression a, b; @@ - a && streq_ptr(a, b) + streq_ptr(a, b)
2b48575
to
1a16c53
Compare
Ooh, you've probably been using this for a while, but I wasn't aware of coccinelle until I read your commit messages. Neat! And, thanks for documenting what you've done. I think coccinelle could be extremely useful for some higher-level transforms of the C code ... it probably would have saved me a lot of work in earlier iterations! Hopefully it can help transform the C code that accesses TeX's structures to allow us to get rid of all of the I see that you've added some I am not a huge fan of the function name And for consistency with other functions, I think I might prefer renaming |
On 20/06/2017 04:28, Peter Williams wrote:
Ooh, you've probably been using this for a while, but I wasn't aware of
coccinelle until I read your commit messages. Neat! And, thanks for
documenting what you've done. I think coccinelle could be *extremely*
useful for some higher-level transforms of the C code ... it probably
would have saved me a lot of work in earlier iterations! Hopefully it
can help transform the C code that accesses TeX's structures to allow us
to get rid of all of the |mem[q].hh.u.B0| type constructs.
Yes, it's a great tool and will be useful for more cleanups in the future :)
I see that you've added some |inline| functions. Are there any
portability dangers associated with this? For the time, I only care
about vaguely recent Linux, OSX, and Windows, so I don't mind if inline
functions are only going to cause problems on old versions of AIX or
whatever.
The only portability issue that comes to mind would be that it requires C99.
I don't think that is an issue. All recent compilers support C99 and all
recent operating systems have a compiler supporting C99.
Also, if we want to improve the quality of the code I don't think we
should restrain ourself to C89.
I am not a huge fan of the function name |streq_ptr| ... is there a
special reasoning to the naming? Please note that there is already a
|#define| called |STREQ| that does the same thing ... if the inline
function is nice and portable, I would definitely prefer to switch to it
to get type-safety etc. But maybe just call it |streq|? Is that name
used on some OS's?
It is called that way because it NULL-checks the pointers, and it checks
if they are equal. An `streq` function would be when you are sure that
the pointers are not NULL, it would only use strcmp without
NULL-checking. In this case I used a conservative approach and
considered that we should check for the pointers each time. We might use
a function that do not check the pointers later if we find some.
In addition, I am used to the naming of some functions from the systemd
project, so that's why I reused such name I guess. And since it
differentiates a function that NULL-checks the pointer or not, I think
it is appropriate.
Do you still want me to change to streq?
Also, like you said I removed the macro because it adds some type
information (and also because I think non-uppercase functions and macro
acting as a function is nicer to look at :)
And for consistency with other functions, I think I might prefer
renaming |startswith| to |strstartswith|.
Sure
|
e1d5a89
to
06a48c6
Compare
Also, what's the purpose of |
|
Use methods for `pdf_color`
Another batch of cleanups for the C code in
tectonic/
.