Closed Bug 1417620 Opened 3 years ago Closed 3 years ago

Port bug 1414067 to mailnews - build/db/mork/src/morkFile.cpp:654:37: error: ignoring return value of 'size_t fwrite(const void*, size_t, size_t, FILE*)', declared with attribute warn_unused_result [-Werror=unused-result]

Categories

(MailNews Core :: Backend, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 59.0

People

(Reporter: jorgk-bmo, Assigned: jorgk-bmo)

Details

Attachments

(2 files, 1 obsolete file)

I'm seeing
build/db/mork/src/morkFile.cpp:654:37: error: ignoring return value of 'size_t fwrite(const void*, size_t, size_t, FILE*)', declared with attribute warn_unused_result [-Werror=unused-result]
on try.

I can't see a bug in this range causing this:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f0c0fb9182d6&tochange=e070277ec199

Can you see which big caused this?
Flags: needinfo?(richard.marti)
Flags: needinfo?(mozilla)
Flags: needinfo?(frgrahl)
Flags: needinfo?(acelists)
This is probably caused by https://bugzilla.mozilla.org/show_bug.cgi?id=1414067

I think this is actually uncovering a bug in our code.
Flags: needinfo?(mozilla)
Flags: needinfo?(acelists)
I'm sorry. I can't help here.
Flags: needinfo?(richard.marti)
Flags: needinfo?(frgrahl)
Summary: Port bug ??? to mailnews - build/db/mork/src/morkFile.cpp:654:37: error: ignoring return value of 'size_t fwrite(const void*, size_t, size_t, FILE*)', declared with attribute warn_unused_result [-Werror=unused-result] → Port bug 1414067 to mailnews - build/db/mork/src/morkFile.cpp:654:37: error: ignoring return value of 'size_t fwrite(const void*, size_t, size_t, FILE*)', declared with attribute warn_unused_result [-Werror=unused-result]
Attached patch 1417620-fwrite.patch (obsolete) — Splinter Review
What do you think of this? That Mork code has been like this forever so I think we can ignore the fwrite() result. Errors are checked right after.

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=db8d4d97488ffc5182691f23a47c33acabc17fda
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Attachment #8928701 - Flags: review?(acelists)
Well, still doesn't compile on Linux, but Mac hasn't failed yet. Looking at M-C code, you see (void), or the error is simply ignored, or we have |Unused << fwrite()| which I tried to avoid in Mork code.

Perhaps I'll just do a
  size_t ignoreMe = fwrite(...)
or is this then going to give an unused variable error?
Comment on attachment 8928701 [details] [diff] [review]
1417620-fwrite.patch

Review of attachment 8928701 [details] [diff] [review]:
-----------------------------------------------------------------

::: db/mork/src/morkFile.cpp
@@ +655,1 @@
>        if ( !ferror(file) )

So why not?
size_t outSize = fwrite(inBuf, 1, inSize, file);
if ( !ferror(file) && outSize == inSize)
Since (void) didn't work for Linux, let's do it the proper way:

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=49e4c4a2043b55fda5252baa3e6e973a3aa65c15
Attachment #8928701 - Attachment is obsolete: true
Attachment #8928701 - Flags: review?(acelists)
Attachment #8928725 - Flags: review?(acelists)
(In reply to :aceman from comment #5)
> So why not?
> size_t outSize = fwrite(inBuf, 1, inSize, file);
> if ( !ferror(file) && outSize == inSize)
We discussed this on chat. That won't work.

      outSize = fwrite(inBuf, 1, inSize, file);
      if ( !ferror(file)  && outSize == inSize)
        outCount = inSize;
      else
        this->new_stdio_file_fault(ev);
will send us to new_stdio_file_fault() which reads errno. So if ferror() returned false, there won't be any errno to act on.

The only thing you can do if you don't trust the fwrite() implementation:
  outSize = fwrite(inBuf, 1, inSize, file);
  MOZ_ASSERT(outSize == inSize || ferror(file));
But that's needless paranoia. The code has been around for ages and no one ever bothered to look at the return value since the error *IS* checked.
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/41bc780f0d2e
fix compiler warning caused by bug 1414067. rs=bustage-fix DONTBUILD
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Comment on attachment 8928725 [details] [diff] [review]
1417620-fwrite.patch (v2)

Review of attachment 8928725 [details] [diff] [review]:
-----------------------------------------------------------------

Compiles for me on Linux.
Attachment #8928725 - Flags: review?(acelists) → review+
Sorry, I don't have the luxury to wait here, the next bustage already arrived and it's getting messy with three bustages at the same time.

The target needs to be corrected to TB 59.
Whiteboard: [PLR]
Target Milestone: --- → Thunderbird 58.0
Thanks, also compiles on Windows, so fingers crossed.
Whiteboard: [PLR]
Does this compile for you?
Attachment #8928763 - Flags: review?(acelists)
Target Milestone: Thunderbird 58.0 → Thunderbird 59.0
Comment on attachment 8928763 [details] [diff] [review]
1417620-linux.patch

Review of attachment 8928763 [details] [diff] [review]:
-----------------------------------------------------------------

Compiles on Linux.
Attachment #8928763 - Flags: review?(acelists) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/8885e04ebabd
follow-up (Linux only): fix compiler warning caused by bug 1414067. r=aceman DONTBUILD
You need to log in before you can comment on or make changes to this bug.