Fix warn_unused_result error in parquet test by karthikeyann · Pull Request #11026 · NVIDIA/cudf

karthikeyann · 2022-06-02T02:39:20Z

Fix error: ignoring return value of declared with attribute warn_unused_result in cpp/tests/io/parquet_test.cpp

codecov · 2022-06-02T04:24:48Z

Codecov Report

❗ No coverage uploaded for pull request base (branch-22.08@c01a2a4). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 8e025af differs from pull request most recent head 85c1199. Consider uploading reports for the commit 85c1199 to get more accurate results

@@               Coverage Diff               @@
##             branch-22.08   #11026   +/-   ##
===============================================
  Coverage                ?   86.34%           
===============================================
  Files                   ?      144           
  Lines                   ?    22710           
  Branches                ?        0           
===============================================
  Hits                    ?    19608           
  Misses                  ?     3102           
  Partials                ?        0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c01a2a4...85c1199. Read the comment docs.

bdice

I have a couple questions but otherwise LGTM for a tiny fix.

Does this only show up on certain compilers?

ttnghia · 2022-06-02T13:19:15Z

  int fd = open(filepath.c_str(), O_RDONLY);
  unsigned char buf[1024];
-  read(fd, buf, sizeof(buf));
+  (void)!read(fd, buf, sizeof(buf));


How about [[maybe_unused]]?

I think it is better to use [[maybe_unused]] instead of the old casting style, if it works.

Should we verify the number of bytes read actually match what we expected? Instead of trying to sidestep the return value, why don't we actually use it?

First, I tried [[maybe_unused]]. It didn't work.
cppreference says, This attribute can appear in the declaration of the following entities
So, it can be used only in declaration.

Right usage is std::ignore

PointKernel · 2022-06-02T13:25:01Z

Thanks for fixing this. I'm thinking if we should replace the c-style system functions (e.g. open, lseek) with std::ifstream which would be more readable IMO and easier to maintain.

ttnghia · 2022-06-02T14:23:20Z

  int fd = open(filepath.c_str(), O_RDONLY);
  unsigned char buf[1024];
-  read(fd, buf, sizeof(buf));
+  (void)!read(fd, buf, sizeof(buf));


I think it is better to use [[maybe_unused]] instead of the old casting style, if it works.

hyperbolic2346 · 2022-06-02T15:24:28Z

  int fd = open(filepath.c_str(), O_RDONLY);
  unsigned char buf[1024];
-  read(fd, buf, sizeof(buf));
+  (void)!read(fd, buf, sizeof(buf));


Should we verify the number of bytes read actually match what we expected? Instead of trying to sidestep the return value, why don't we actually use it?

hyperbolic2346 · 2022-06-02T15:25:52Z

  int fd = open(filepath.c_str(), O_RDONLY);
  unsigned char buf[1024];
-  read(fd, buf, sizeof(buf));
+  (void)!read(fd, buf, sizeof(buf));


Suggested change

(void)!read(fd, buf, sizeof(buf));

size_t read_bytes = read(fd, buf, sizeof(buf));

EXPECT_EQ(read_bytes, sizeof(buf));

It should read 1024 bytes unless the file is shorter or there is an error. This is why read returns the number of bytes actually read so you don't interpret garbage as real data.

No, read_bytes is the actual size of read file, while sizeof(buff) is 1024. Do we know the file size beforehand?

It should read 1024 bytes unless the file is shorter or there is an error.

Do we know the file size is >1024?

Ah I see, file size should be bigger. But this is error-prone since we may change the file size generated above at any time.

I agree that this logic could be made cleaner with c++ streams but for now, I don't see the test breaking on its own. Unless, of course, if the read fails altogether for some reason. @etseidl (author) can confirm.

Sorry, coming late to the game...I have changes to this test pending in a separate branch. PR will be coming as soon as I get work approval. I've completely reworked this test to use the cudf::io::datasource interface with the CompactProtocolReader.

Sorry for the trouble!

@hyperbolic2346 @PointKernel I can cherry-pick out the changes for that test pretty easily. Should I do a smaller PR to fix this (hopefully) for good?

If it is coming in 22.08, I wouldn't bother to pull it out.

I am using c++ (Ubuntu 9.4.0-1ubuntu1~18.04) 9.4.0 in my local machine

I cannot replicate in godbolt. But can replicate same error in my local machine compilation
g++ -fdiagnostics-color=always -Wno-deprecated-declarations -O3 -DNDEBUG -fPIE -Wall -Werror -Wno-unknown-pragmas -Wno-error=deprecated-declarations -Wno-deprecated-declarations -pthread -std=gnu++17 test.cpp

#include <string> #include <unistd.h> #include <fcntl.h> int main() { std::string filepath = "CheckPageRows.parquet"; int fd = open(filepath.c_str(), O_RDONLY); unsigned char buf[1024]; read(fd, buf, sizeof(buf)); return 0; }

test.cpp: In function ‘int main()’: test.cpp:9:10: error: ignoring return value of ‘ssize_t read(int, void*, size_t)’, declared with attribute warn_unused_result [-Werror=unused-result] 9 | read(fd, buf, sizeof(buf)); | ~~~~^~~~~~~~~~~~~~~~~~~~~~ cc1plus: all warnings being treated as errors

same error with (void). Only with (void)! no errors.

(base) yunsongw@yunsongw-dt:~/Work$ gcc --version gcc (Ubuntu 9.3.0-17ubuntu1~20.04) 9.3.0 Copyright (C) 2019 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. (base) yunsongw@yunsongw-dt:~/Work$ g++ -Werror -O toto.cxx toto.cxx: In function ‘int main()’: toto.cxx:9:10: error: ignoring return value of ‘ssize_t read(int, void*, size_t)’, declared with attribute warn_unused_result [-Werror=unused-result] 9 | read(fd, buf, sizeof(buf)); | ~~~~^~~~~~~~~~~~~~~~~~~~~~ cc1plus: all warnings being treated as errors

-Werror -O is the minimum to replicate the error on my local system but I cannot reproduce the same error with any of the godbolt compiler versions (https://godbolt.org/z/f5aTzM7fx).

hyperbolic2346 · 2022-06-02T15:42:06Z

Thanks for fixing this. I'm thinking if we should replace the c-style system functions (e.g. open, lseek) with std::ifstream which would be more readable IMO and easier to maintain.

I would file an issue to track that and leave this PR as a quick fix. I assume that was your suggestion here, but I wanted to be sure it was spelled out. I don't want to burden @karthikeyann with that when this would be a better situation than we have now and it is ready to go.

PointKernel · 2022-06-02T15:49:59Z

I would file an issue to track that and leave this PR as a quick fix.

Works for me. Thanks for doing this!

Edits: Didn't pay enough attention to Mike's comment. Thought you had opened the issue already.

PointKernel · 2022-06-03T14:12:44Z

I think we can merge the current PR as it is right now since @etseidl already has a fix. I've opened #11038 to track the c++ refactoring/cleanup separately.

Mike said it was okay to leave this as a quick fix in a comment.

bdice · 2022-06-03T15:11:15Z

I dismissed the requests for changes and I'm going to merge this small fix. I will ask for forgiveness instead of permission on this one, if that's alright. It's blocking me and it has multiple approvals (both ✔️ and verbal support in comments). We have an open issue #11038 to track further improvements and planned changes in a future PR from @etseidl.

bdice · 2022-06-03T15:11:27Z

@gpucibot merge

davidwendt · 2022-06-03T18:09:42Z

But what about that exclamation point? Is (void)! like I saying force cast this? That sounds like Jedi C++

bdice · 2022-06-03T19:27:24Z

@davidwendt I think the links in this thread should help answer your question about (void)!. It seems to be a compiler bug? #11026 (comment).

hyperbolic2346 · 2022-06-04T02:26:56Z

But what about that exclamation point? Is (void)! like I saying force cast this? That sounds like Jedi C++

The ! is negating the return and then throwing it away. This makes the compiler happy because you technically used the return value.

ttnghia · 2022-06-04T02:31:43Z

So why not just !read(...)? That's confusing 😃

fix unused result error

85c1199

karthikeyann added bug Something isn't working 3 - Ready for Review Ready for review by team tests Unit testing for project libcudf Affects libcudf (C++/CUDA) code. 4 - Needs Review Waiting for reviewer to review or respond non-breaking Non-breaking change labels Jun 2, 2022

karthikeyann self-assigned this Jun 2, 2022

karthikeyann requested a review from a team as a code owner June 2, 2022 02:39

karthikeyann requested review from codereport, devavret, hyperbolic2346 and trxcllnt June 2, 2022 02:39

karthikeyann changed the title ~~Fix warn_unused_result error~~ Fix warn_unused_result error in parquet test Jun 2, 2022

devavret approved these changes Jun 2, 2022

View reviewed changes

bdice approved these changes Jun 2, 2022

View reviewed changes

Comment thread cpp/tests/io/parquet_test.cpp

ttnghia reviewed Jun 2, 2022

View reviewed changes

ttnghia previously requested changes Jun 2, 2022

View reviewed changes

hyperbolic2346 previously requested changes Jun 2, 2022

View reviewed changes

hyperbolic2346 reviewed Jun 2, 2022

View reviewed changes

PointKernel approved these changes Jun 3, 2022

View reviewed changes

rapids-bot Bot merged commit f6c451b into NVIDIA:branch-22.08 Jun 3, 2022

	(void)!read(fd, buf, sizeof(buf));
	size_t read_bytes = read(fd, buf, sizeof(buf));
	EXPECT_EQ(read_bytes, sizeof(buf));

Uh oh!

Conversation

karthikeyann commented Jun 2, 2022

Uh oh!

codecov Bot commented Jun 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bdice 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!

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!

PointKernel commented Jun 2, 2022

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!

ttnghia Jun 2, 2022 • 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!

karthikeyann Jun 5, 2022 • 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!

hyperbolic2346 commented Jun 2, 2022

Uh oh!

PointKernel commented Jun 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PointKernel commented Jun 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bdice commented Jun 3, 2022

Uh oh!

bdice commented Jun 3, 2022

Uh oh!

davidwendt commented Jun 3, 2022

Uh oh!

bdice commented Jun 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hyperbolic2346 commented Jun 4, 2022

Uh oh!

ttnghia commented Jun 4, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

codecov Bot commented Jun 2, 2022 •

edited

Loading

bdice left a comment •

edited

Loading

ttnghia Jun 2, 2022 •

edited

Loading

karthikeyann Jun 5, 2022 •

edited

Loading

PointKernel commented Jun 2, 2022 •

edited

Loading

PointKernel commented Jun 3, 2022 •

edited

Loading

bdice commented Jun 3, 2022 •

edited

Loading