-
Notifications
You must be signed in to change notification settings - Fork 162
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
Fix warnings in tectonic/ #66
Conversation
Nice. Looks good to me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for undertaking this, and so quickly too! I was not looking forward to slogging through all of the warnings that I just activated.
I have a couple of questions about the changes, and then there is one change that changes the semantics of the code and so needs to be checked over.
build.rs
Outdated
.flag("-Wunreachable-code") | ||
.flag("-Wstrict-prototypes") | ||
.flag("-Wold-style-definition") | ||
.flag("-Wwrite-strings") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am concerned that these options could cause problems for platforms where the compiler is not actually gcc
. (While the Rust crate is named gcc
, it will use whatever system C compiler it thinks best.) Is there a way to test whether the compiler is actually gcc
in particular, and only add these flags if we can confirm that it is? If not, I would rather not apply all of these extra flags, although I'm willing to be convinced otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK, the rust crate does not allow to test if a flag works or does not currently provide a way to implement such test.
Do you prefer to remove the additional flags and wait that the rust crate implements an API to do so? Do you prefer to check first if those flags are supported by other compilers? if so which ones (clang or another one?)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is better to remove the extra flags for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I removed the commit from the branch.
tectonic/XeTeXFontInst.cpp
Outdated
@@ -315,6 +315,9 @@ XeTeXFontInst::initialize(const char* pathname, int index, int &status) | |||
TT_OS2* os2Table; | |||
FT_Error error; | |||
hb_face_t *hbFace; | |||
size_t sz; | |||
ssize_t r; | |||
FT_Byte *data; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Late-time variable declarations are legal in C++, I believe, and they're kind of nice, so I would rather leave them where they were before in this function, unless there's a reason to move them up that I'm missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. It is just an habit, in my case I prefer to have variables declared at the beginning of the current scope :) I will go back to the previous declaration.
@@ -69,7 +71,7 @@ dvipdfmx_simple_main(tt_bridge_api_t *api, char *dviname, char *pdfname) | |||
{ | |||
extern int dvipdfmx_main(int argc, char *argv[]); | |||
|
|||
char *argv[] = { "dvipdfmx", "-o", pdfname, dviname }; | |||
char *argv[] = { xstrdup("dvipdfmx"), xstrdup("-o"), pdfname, dviname }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for being lazy here — why are these xstrdup
s needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const char*
was being assigned to char*
. So there is a potential that the program might try to change the static strings.
Actually, wouldn't just declaring argv
with const char *argv[]
solve this since argv
is not used to mutate the content in the code as far as I can tell? This would need also dvipdfmx_main
to be declared with const char* argv
, or would just casting be enough like in dvipdfmx_main(4, (char**) argv)
(I'm not sure if this is the correct syntax for the cast)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, yes, that was my hunch. I believe that, yes, this might be fixable by const-ifying the relevant variables rather than doing xstrdup
s. If so, I think it would be much better to const-ify things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not possible to use a const. dvipdfmx_main
calls do_early_args
and do_args
, which call getopt_long
. This function requires a char * const argv[]
.
The only solution would be to avoid the allocation would be to do an explicit cast. I'm not sure this is the best approach. A better approach would be to fix the code on its own and not have a function that expect argc
and argv
(I think such significant change belongs to another PR).
Hence, my solution was to use xstrdup
which is used elsewhere in such a case. I don't think two additional allocations will have an impact on the performance. It also fixes the issue without using an explicit cast that hide the issue (i.e., any function inside dvipdfmx_main
could still try to modify the content of argv[0]
or argv[1]
resulting in a crash).
tectonic/dpx-fontmap.c
Outdated
@@ -1281,7 +1281,7 @@ strip_options (const char *map_name, fontmap_opt *opt) | |||
return font_name; | |||
} | |||
|
|||
#if DPXTEST | |||
#ifdef DPXTEST |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole chunk of code can just be deleted because we are not in fact ever going to #define DPXTEST
.
tectonic/dpx-pdfobj.c
Outdated
@@ -1693,8 +1693,9 @@ apply_filter_TIFF2_1_2_4 (unsigned char *raster, | |||
} | |||
} | |||
} | |||
if (outbits > 0) | |||
if (outbits > 0) { | |||
raster[k] = (outbuf << (8 - outbits)); k++; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yikes! This changes the semantics of the code — the brace-less if
only applies to one statement, so the k++
is executed regardless of the result of the if
test. It would be good to give this function a careful read and figure out what the correct behavior is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case k++;
can be safely removed because the variable only ever gets set after the increment operation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some other questionable lines of code though that'll need deeper understanding to be fix with confidence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How did that happen? In xetex.web
the line is cur_val1:=cur_val1 + cur_val * @"200000;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That line from xetex.web
probably results in line 23163 in xetex0.c.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in #72
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I will add another commit removing k++;
then. Thanks :)
tectonic/dpx-subfont.c
Outdated
@@ -474,7 +474,7 @@ release_sfd_record (void) | |||
} | |||
|
|||
|
|||
#if DPXTEST | |||
#ifdef DPXTEST |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto about just deleting the code chunk.
@@ -949,7 +949,7 @@ pdf_font_load_truetype (pdf_font *font) | |||
return -1; | |||
} | |||
|
|||
#if ENABLE_NOEMBED | |||
#ifdef ENABLE_NOEMBED |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, the same consideration may apply here, but I don't know what ENABLE_NOEMBED
does, so I'm not sure whether we want to act as if it is defined, or not. Out of scope for this PR, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I will leave this code then.
@@ -3736,7 +3736,7 @@ tt_run_engine(char *dump_name, char *input_file_name) | |||
/* Make this something invariant so that we can use XDV files to test | |||
* reproducibility of the engine output. */ | |||
|
|||
output_comment = "tectonic"; | |||
output_comment = xstrdup("tectonic"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note-to-self that this is another xstrdup
I don't understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could probably be also solved by declaring output_comment
as const char *
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, currently, it is not possible. In engine-interface.c, output_comment
is allocated. Only here it is a constant. Thus, I use xstrdup
.
I think it could be fixed but differently, like not using a global variable. However, it requires more significant changes. Another PR improving the code by removing global variables would be more appropriate.
By the way, the Travis failure is something unrelated to this PR — I will work on fixing that separately. |
If you rebase or merge this PR with the latest |
-Wignore-qualifier
It is needed by the following files: tectonic/dpx-cff_types.h:24:#ifdef HAVE_INTTYPES_H tectonic/dpx-dpxcrypt.h:25:#ifdef HAVE_INTTYPES_H tectonic/dpx-mem.h:27:#ifdef HAVE_INTTYPES_H tectonic/dpx-numbers.h:28:#ifdef HAVE_INTTYPES_H Since other headers, such as stdint.h, are assumed to exist, we assume the same for inttypes.h.
DPXTEST and ENABLE_NOEMBED are not defined by default, the #if are meant to be a #ifdef.
-Wdiscarded-qualifiers
-Wformat -Wformat-extra-args -Wformat-security
-Wold-style-definition -Wstrict-prototypes
-Wredundant-decls
No caller check the return value of the function and the function does not return anything. So we change int to void. -Wreturn-type
-Wmisleading-indentation
ababa0a
to
9ea9d3b
Compare
@pkgw Rebased and applied fixes related to most of the comments. I did not remove the additional flags yet, I am waiting for your comment on that. I also did not change how I fix the assignation of const to a non-const variable. I think it fixes temporarily the issue properly better than casting away the value which just hide the issue, if you think not I can fix the commit to cast the value. A better fix obviously would be to improve the overall quality of the code, but I guess this will be done incrementally :) |
@ronnychevalier Edit: sorry, wrote this before reading your replies above — the comment below is basically inoperative, I see. Original comment: For the const-to-non-const assignments, the idea we were discussing was to make the destination variables const as well — I think this should be doable without requiring too many changes. If that's not the case, though, we can just take your approach for now and fix up later. |
|
We will never use/define DPXTEST, hence remove the dead code.
k is reassigned on the next iteration of the loop. Hence, the k++ is useless. Reported by @Mrmaxmeier
9ea9d3b
to
90d36f1
Compare
Right, sorry, I wrote my first reply before seeing and reading your replies inline with the code review. With your explanations, I think this will be good to merge once the extra compiler flags are removed. (For the time being — I would certainly like to enable lots of warnings in general! But I don't want to accidentally break the build on unusual OS setups.) |
pdf_coord::new, bmp_header on slices
This PR starts fixing some warnings. It also adds some additional warning flags.
The only remaining "important" warnings of
-Wall
to fix are those related to-Wpointer-sign
, otherwise it is only unused parameters/variables. I added a commit to disable-Wpointer-sign
until these warnings are fixed. It avoids spamming the build output with warnings related totectonic/
code.