Open Bug 1628542 Opened 4 years ago Updated 2 months ago

Remove mozilla::Unused and unused_t

Categories

(Core :: MFBT, task, P3)

task

Tracking

()

People

(Reporter: cpeterson, Unassigned)

References

(Depends on 2 open bugs, Blocks 1 open bug)

Details

(Keywords: good-first-bug)

Attachments

(2 files)

In bug 1059038 comment 7, Waldo said:

I think people should 1) not ignore return values, at the least to assert some characteristic about them, or failing that, 2) just cast to void.

mozilla::Unused was needed because casting to void doesn't suppress gcc's warn_unused_result warnings. It does suppress gcc's and clang's warnings about C++17 [[nodiscard]]. Once we globally replace MOZ_MUST_USE with [[nodiscard]] (in bug 1571631), we can replace uses of Unused << with (void) casts like:

- Unused << SendShowEvent(aData, aFromUser);
+ (void)SendShowEvent(aData, aFromUser);

However, a safer way to ignore return values is to assert function calls that "can never fail" using MOZ_ALWAYS_TRUE()/etc. MOZ_ALWAYS_TRUE(X) evaluates the expression X in both debug and release builds, so you could write:

- Unused << SendShowEvent(aData, aFromUser);
+ MOZ_ALWAYS_TRUE(SendShowEvent(aData, aFromUser));

TBD: Can we also remove MOZ_UNUSED (bug 1202965)? It was added for use in C code. There are only two uses of MOZ_UNUSED at this time (suppressing gcc warnings about unused write() return values that can't be suppressed by casting to void):

https://searchfox.org/mozilla-central/search?case=true&q=MOZ_UNUSED

We could replace them with something like (in order of increasing pessimism);

  • MOZ_ALWAYS_TRUE(write(fd, buf, nbytes) == nbytes));
  • MOZ_ALWAYS_TRUE(write(fd, buf, nbytes) >= 0));
  • MOZ_ALWAYS_TRUE(write(fd, buf, nbytes) >= -1));

(In reply to Chris Peterson [:cpeterson] from comment #0)

TBD: Can we also remove MOZ_UNUSED (bug 1202965)? It was added for use in C code. There are only two uses of MOZ_UNUSED at this time (suppressing gcc warnings about unused write() return values that can't be suppressed by casting to void):

I wonder if there is a reason why FdPrintf() at least doesn't handle EINTR.

build/clang-plugin/KungFuDeathGripChecker.cpp must also be updated, since its diagnostics suggest the use of mozilla::Unused.

  • MOZ_ALWAYS_TRUE(write(fd, buf, nbytes) == nbytes));

This is wrong because write has no guarantee to write all requested bytes at once. We have to repeat ourselves to ensure that all requested bytes are written.

Depends on: 1713212
No longer blocks: 1059038
Severity: normal → --
See Also: → 1059038
Keywords: good-first-bug
Blocks: 1802907
Assignee: nobody → bpostelnicu
Status: NEW → ASSIGNED

Depends on D173711

Assignee: bpostelnicu → nobody
Status: ASSIGNED → NEW
Assignee: nobody → bpostelnicu
Status: NEW → ASSIGNED

Looking at my patch in bug 1826734, I think any Unused << nsresult cases should be converted to something that logs a warning if NS_FAILED. And also for (void)nsresult.

dom/quota has a great helper function named QM_WARNONLY_TRY, it would be great to use that here.

Depends on: 1672378

This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit BugBot documentation.

Assignee: bpostelnicu → nobody
Status: ASSIGNED → NEW
See Also: → 1889080
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: