Compile with -Werror and -Wall #1116

robertnishihara · 2017-10-12T20:20:19Z

This isn't catching as many things as I would like (e.g., it didn't find anything in our Python C extension code, it didn't complain about implicit casts or C-style casts or anything like that. Are there more flags I should add?

#1087

atumanov · 2017-10-12T20:28:55Z

Try -Wextra .

atumanov · 2017-10-12T20:32:43Z

for catching C style casts : -Wold-style-cast (not included in -Wall or -Wextra)

mehrdadn · 2017-10-12T20:34:07Z

On GCC it's impossible to enable all warnings with a generic flag; the most you can do is -Wall -Wextra -pedantic. On Clang there's -Weverything. Not all warnings have merit though, so you might get false positives.

And note that you should not enable -Werror with -Weverything (and technically not with -Wall or -Wextra either, but the very reason we have so many flags is that too many people did so and then complained that their builds broke, so the compiler maintainers added new flags instead of letting -Wall stay true to its name). You should only treat warnings that you're aware of as errors, so that the build doesn't break with newer versions of the compiler. (They'll still be there as warnings obviously, so you won't miss them.)

Oh, and if you want to get a list of all warnings, try the following:

GCC: gcc -Q --help=warning
Clang: http://clang.llvm.org/docs/DiagnosticsReference.html

AmplabJenkins · 2017-10-12T20:37:35Z

Merged build finished. Test FAILed.

AmplabJenkins · 2017-10-12T20:37:36Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/2107/
Test FAILed.

AmplabJenkins · 2017-10-12T20:59:19Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-10-12T20:59:20Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/2106/
Test PASSed.

mehrdadn · 2017-10-12T21:10:44Z

src/common/io.cc

@@ -369,7 +369,7 @@ int64_t read_vector(int fd, int64_t *type, std::vector<uint8_t> &buffer) {
  if (closed) {
    goto disconnected;
  }
-  if (length > buffer.size()) {
+  if (static_cast<size_t>(length) > buffer.size()) {


Why not just have length be uint64_t (I'm assuming size_t is not an option)?
It's better to let the data type match the semantics of the variable than to check for it. Otherwise if you pass around signed variables for unsigned quantities then you'll keep having to worry about whether a negative value is possible and if it might have a special meaning.

mehrdadn · 2017-10-12T21:11:31Z

src/common/io.cc

@@ -369,7 +369,7 @@ int64_t read_vector(int fd, int64_t *type, std::vector<uint8_t> &buffer) {
  if (closed) {
    goto disconnected;
  }
-  if (length > buffer.size()) {
+  if (static_cast<size_t>(length) > buffer.size()) {
    buffer.resize(length);


This would "work" but it's very confusing and wouldn't make sense if length is negative. There should at least be a cast here. But see my comment above.

mehrdadn · 2017-10-12T21:15:07Z

src/common/task.cc

      /* Make sure the resource vector is constructed entry by entry,
       * in order. */
-      CHECK(resource_index == resource_vector_.size());
+      CHECK(static_cast<size_t>(resource_index) == resource_vector_.size());
      resource_vector_.resize(resource_index + 1);


This is especially confusing... are you adding 1 to a potentially negative value? Is that what you intend?
(Note that the check is indeed above, but the code should make sense without the CHECKs.)

Are you saying that in general casting from signed to unsigned is bad? E.g., would it be better to do the following?

CHECK(resource_index == static_cast<int64_t>(resource_vector_.size()));

No, I'm saying (like in the previous comment) the variable should be unsigned. Right now you're adding 1 to a signed value and then resizing based on that, which doesn't make sense in the negative case.

So in our case we have decided to make all index variables signed and we try to keep it that way; this is also done by others like Google and Arrow so we are in good company. The reason is that it simplifies arithmetic and guards against the variable wrapping around zero (it is easier to detect a negative index than a large positive one).

Signed values don't actually simplify anything. (?) They only make it complicated to range-check, since now you have to check both < 0 and >= size() rather than just the latter. I'm also not sure what you mean about "guarding" against variables wrapping around zero—adding (ptrdiff_t)(-1) and adding ~(size_t)0 to a pointer p results in exactly the same address p - 1 regardless of which one you choose. In fact, if you have checked pointers enabled (_GLIBCXX_DEBUG may do this? not sure), the checking may actually catch large unsigned values since those would always be illegal even if they were to coincidentally land inside the array, but it cannot possibly catch bitwise-identical negative values if they happen to point inside the array.

Moreover, what there is a difference in is that overflowing signed values is in fact undefined rather than well-defined behavior. So that means your code could do all sorts of crazy things if you in fact do happen to accidentally overflow, rather than merely writing to a lower spot than the one you intended to.

More generally, I would not hold Google as the example of ideal C++ coding. They have guidelines that are pretty universally understood to be often bad advice (no need to take my word for this; just Google around...), and tailored to their own particular use cases. I don't know why Arrow might do this but I'm sure you won't have trouble finding people who use unsigned values either... =P

That said, you obviously don't have to change this if you'd rather not. Just letting you know what the issues are.

AmplabJenkins · 2017-10-12T23:05:13Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-10-12T23:05:13Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/2108/
Test PASSed.

pcmoritz

+1 LGTM

robertnishihara · 2017-10-13T04:01:19Z

Thanks @mehrdadn and @atumanov.

robertnishihara and others added 7 commits October 12, 2017 12:52

Compile global scheduler with -Werror -Wall.

1b65490

Compile plasma manager with -Werror -Wall.

a9fac12

Compile local scheduler with -Werror -Wall.

f36f5de

Compile common code with -Werror -Wall.

d91c08b

Signed/unsigned comparisons.

ddc9c2f

More signed/unsigned fixes.

8d51c79

More signed/unsigned fixes and added extern keyword.

573ec8b

robertnishihara requested a review from mehrdadn October 12, 2017 20:20

Fix linting.

804aeac

mehrdadn reviewed Oct 12, 2017

View reviewed changes

Don't check strict-aliasing because Python.h doesn't pass.

7801c73

pcmoritz self-requested a review October 13, 2017 03:59

pcmoritz approved these changes Oct 13, 2017

View reviewed changes

pcmoritz merged commit 486cb64 into ray-project:master Oct 13, 2017

pcmoritz deleted the warningserrors branch October 13, 2017 04:00

robertnishihara mentioned this pull request Oct 13, 2017

Turn compiler warnings into errors. #1087

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compile with -Werror and -Wall #1116

Compile with -Werror and -Wall #1116

robertnishihara commented Oct 12, 2017

atumanov commented Oct 12, 2017

atumanov commented Oct 12, 2017

mehrdadn commented Oct 12, 2017 •

edited

Loading

AmplabJenkins commented Oct 12, 2017

AmplabJenkins commented Oct 12, 2017

AmplabJenkins commented Oct 12, 2017

AmplabJenkins commented Oct 12, 2017

mehrdadn Oct 12, 2017

mehrdadn Oct 12, 2017

mehrdadn Oct 12, 2017 •

edited

Loading

robertnishihara Oct 13, 2017

mehrdadn Oct 13, 2017

pcmoritz Oct 13, 2017 •

edited by robertnishihara

Loading

mehrdadn Oct 13, 2017 •

edited

Loading

AmplabJenkins commented Oct 12, 2017

AmplabJenkins commented Oct 12, 2017

pcmoritz left a comment

robertnishihara commented Oct 13, 2017

Compile with -Werror and -Wall #1116

Compile with -Werror and -Wall #1116

Conversation

robertnishihara commented Oct 12, 2017

atumanov commented Oct 12, 2017

atumanov commented Oct 12, 2017

mehrdadn commented Oct 12, 2017 • edited Loading

AmplabJenkins commented Oct 12, 2017

AmplabJenkins commented Oct 12, 2017

AmplabJenkins commented Oct 12, 2017

AmplabJenkins commented Oct 12, 2017

mehrdadn Oct 12, 2017

Choose a reason for hiding this comment

mehrdadn Oct 12, 2017

Choose a reason for hiding this comment

mehrdadn Oct 12, 2017 • edited Loading

Choose a reason for hiding this comment

robertnishihara Oct 13, 2017

Choose a reason for hiding this comment

mehrdadn Oct 13, 2017

Choose a reason for hiding this comment

pcmoritz Oct 13, 2017 • edited by robertnishihara Loading

Choose a reason for hiding this comment

mehrdadn Oct 13, 2017 • edited Loading

Choose a reason for hiding this comment

AmplabJenkins commented Oct 12, 2017

AmplabJenkins commented Oct 12, 2017

pcmoritz left a comment

Choose a reason for hiding this comment

robertnishihara commented Oct 13, 2017

mehrdadn commented Oct 12, 2017 •

edited

Loading

mehrdadn Oct 12, 2017 •

edited

Loading

pcmoritz Oct 13, 2017 •

edited by robertnishihara

Loading

mehrdadn Oct 13, 2017 •

edited

Loading