Fix warnings in tectonic/ by ronnychevalier · Pull Request #66 · tectonic-typesetting/tectonic

ronnychevalier · 2017-06-04T20:29:57Z

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 to tectonic/ code.

rekka · 2017-06-05T00:04:50Z

Nice. Looks good to me.

pkgw

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.

pkgw · 2017-06-05T00:15:55Z

+        .flag("-Wunreachable-code")
+        .flag("-Wstrict-prototypes")
+        .flag("-Wold-style-definition")
+        .flag("-Wwrite-strings")


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.

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?)?

I think it is better to remove the extra flags for now.

Ok. I removed the commit from the branch.

pkgw · 2017-06-05T00:17:29Z

    hb_face_t *hbFace;
+    size_t sz;
+    ssize_t r;
+    FT_Byte *data;


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.

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.

pkgw · 2017-06-05T00:18:01Z

    extern int dvipdfmx_main(int argc, char *argv[]);

-    char *argv[] = { "dvipdfmx", "-o", pdfname, dviname };
+    char *argv[] = { xstrdup("dvipdfmx"), xstrdup("-o"), pdfname, dviname };


Sorry for being lazy here — why are these xstrdups needed?

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)?

OK, yes, that was my hunch. I believe that, yes, this might be fixable by const-ifying the relevant variables rather than doing xstrdups. If so, I think it would be much better to const-ify things.

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).

pkgw · 2017-06-05T00:19:11Z

 }

-#if  DPXTEST
+#ifdef  DPXTEST


This whole chunk of code can just be deleted because we are not in fact ever going to #define DPXTEST.

pkgw · 2017-06-05T00:20:39Z

        }
-        if (outbits > 0)
+        if (outbits > 0) {
            raster[k] = (outbuf << (8 - outbits)); k++;


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.

In this case k++; can be safely removed because the variable only ever gets set after the increment operation.

There are some other questionable lines of code though that'll need deeper understanding to be fix with confidence.

How did that happen? In xetex.web the line is cur_val1:=cur_val1 + cur_val * @"200000;

That line from xetex.web probably results in line 23163 in xetex0.c.

Fixed in #72

Good catch!

Ok, I will add another commit removing k++; then. Thanks :)

pkgw · 2017-06-05T00:21:13Z



-#if  DPXTEST
+#ifdef  DPXTEST


Ditto about just deleting the code chunk.

pkgw · 2017-06-05T00:22:33Z

    }

-#if  ENABLE_NOEMBED
+#ifdef  ENABLE_NOEMBED


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.

Ok I will leave this code then.

pkgw · 2017-06-05T00:23:14Z

     * reproducibility of the engine output. */

-    output_comment = "tectonic";
+    output_comment = xstrdup("tectonic");


Note-to-self that this is another xstrdup I don't understand.

This could probably be also solved by declaring output_comment as const char *.

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.

pkgw · 2017-06-05T00:25:33Z

By the way, the Travis failure is something unrelated to this PR — I will work on fixing that separately.

pkgw · 2017-06-05T01:04:19Z

If you rebase or merge this PR with the latest master, the Travis CI run should be able to succeed.

-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

-Wsign-compare

-Wmisleading-indentation

ronnychevalier · 2017-06-06T18:26:42Z

@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 :)

pkgw · 2017-06-06T18:51:15Z

@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.

ronnychevalier · 2017-06-06T18:55:21Z

output_comment cannot be const, as explained in my previous message, since it is a global variable that is set dynamically in engine-interface.c. For argv it cannot be const as well since by following the trail of functions that use it, the final getopt_long function in glibc does not expect a const. So we would have to cast it from const to non-const.

We will never use/define DPXTEST, hence remove the dead code.

@Mrmaxmeier

k is reassigned on the next iteration of the loop. Hence, the k++ is useless. Reported by @Mrmaxmeier

pkgw · 2017-06-06T18:57:58Z

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

pkgw requested changes Jun 5, 2017

View reviewed changes

ronnychevalier added 10 commits June 6, 2017 18:11

tectonic/core-bridge: remove useless const qualifier

77a7038

-Wignore-qualifier

tectonic/dpx: replace #if to #ifdef

3876e2b

DPXTEST and ENABLE_NOEMBED are not defined by default, the #if are meant to be a #ifdef.

tectonic: avoid assigning const value to non const variable

3cae115

-Wdiscarded-qualifiers

tectonic: fix many format issues

90f2ab4

-Wformat -Wformat-extra-args -Wformat-security

tectonic: fix warnings related to old style definitions

f79171f

-Wold-style-definition -Wstrict-prototypes

tectonic: fix redundant reclarations warnings

3fa691c

-Wredundant-decls

tectonic/bibtex: fix wrong return type

ed6f5d5

No caller check the return value of the function and the function does not return anything. So we change int to void. -Wreturn-type

tectonic: fix warnings due to comparison of signed/unsigned values

014a411

-Wsign-compare

tectonic: fix warnings about misleading indentation

9e70f6e

-Wmisleading-indentation

ronnychevalier force-pushed the rc/warnings branch from ababa0a to 9ea9d3b Compare June 6, 2017 18:22

ronnychevalier added 2 commits June 6, 2017 20:56

tectonic/dpx: remove useless code related to DPXTEST

55bd3dd

We will never use/define DPXTEST, hence remove the dead code.

tectonic/dpx-pdfobj: remove useless assignation

90d36f1

k is reassigned on the next iteration of the loop. Hence, the k++ is useless. Reported by @Mrmaxmeier

ronnychevalier force-pushed the rc/warnings branch from 9ea9d3b to 90d36f1 Compare June 6, 2017 18:56

pkgw approved these changes Jun 6, 2017

View reviewed changes

pkgw merged commit c85e179 into tectonic-typesetting:master Jun 6, 2017

Mrmaxmeier pushed a commit to Mrmaxmeier/tectonic that referenced this pull request Oct 1, 2019

Merge pull request tectonic-typesetting#66 from burrbull/coodr

02dd92a

pdf_coord::new, bmp_header on slices

Uh oh!

Conversation

ronnychevalier commented Jun 4, 2017

Uh oh!

rekka commented Jun 5, 2017

Uh oh!

pkgw left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pkgw Jun 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pkgw Jun 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pkgw Jun 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pkgw commented Jun 5, 2017

Uh oh!

pkgw commented Jun 5, 2017

Uh oh!

ronnychevalier commented Jun 6, 2017

Uh oh!

pkgw commented Jun 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ronnychevalier commented Jun 6, 2017

Uh oh!

pkgw commented Jun 6, 2017

Uh oh!

Reviewers

pkgw left a comment •

edited

Loading

pkgw Jun 5, 2017 •

edited

Loading

pkgw Jun 5, 2017 •

edited

Loading

pkgw Jun 5, 2017 •

edited

Loading

pkgw commented Jun 6, 2017 •

edited

Loading