Page MenuHomePhabricator

mbenfield (Michael Benfield)
User

Projects

User does not belong to any projects.

User Details

User Since
Apr 13 2021, 1:34 PM (4 w, 5 d)

Recent Activity

Wed, May 5

mbenfield requested review of D101937: [Clang][Manual] Mention -fdebug-info-for-profiling..
Wed, May 5, 12:53 PM · Restricted Project

Fri, Apr 30

mbenfield added a comment to D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable.

FWIW gcc does not warn in this case regardless of any cast:

Fri, Apr 30, 7:06 PM · Restricted Project
mbenfield added a comment to D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable.

I see lots of instances from the kernel that look like this when reduced:

Fri, Apr 30, 4:37 PM · Restricted Project
mbenfield updated the diff for D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable.

Count a non-ODR use as a reference, so that examples like that of nickdesaulniers don't warn.

Fri, Apr 30, 3:42 PM · Restricted Project
mbenfield updated the diff for D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable.

Correct struct and vector behavior.

Fri, Apr 30, 2:16 PM · Restricted Project
mbenfield added a comment to D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable.

Apparently arc and/or Phabricator doesn't like my attempt to split this review into two separate commits. Next revision will have only one commit.

Fri, Apr 30, 1:35 PM · Restricted Project
mbenfield updated the diff for D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable.

A second commit with UnusedButSetParameter and UnusedButSetVariable into groups.

Fri, Apr 30, 10:25 AM · Restricted Project

Thu, Apr 29

mbenfield added a comment to D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable.

Will also make a new revision tomorrow with these warnings added to the appropriate groups.

Thu, Apr 29, 4:50 PM · Restricted Project
mbenfield added a comment to D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable.
Thu, Apr 29, 4:49 PM · Restricted Project
mbenfield updated the diff for D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable.

Another try at these warnings, using the implementation strategy outlined by rsmith.

Thu, Apr 29, 4:09 PM · Restricted Project

Wed, Apr 28

mbenfield abandoned D101480: Revert "[Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable".
Wed, Apr 28, 3:06 PM · Restricted Project
mbenfield added a comment to D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable.
Wed, Apr 28, 1:57 PM · Restricted Project
mbenfield added a comment to D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable.

So it seems there are 3 issues here:

Wed, Apr 28, 1:24 PM · Restricted Project
mbenfield added a comment to D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable.
Wed, Apr 28, 12:12 PM · Restricted Project
mbenfield added a reverting change for rG9b0501abc7b5: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable: D101480: Revert "[Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable".
Wed, Apr 28, 12:12 PM
mbenfield requested review of D101480: Revert "[Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable".
Wed, Apr 28, 12:12 PM · Restricted Project
mbenfield added a reverting change for D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable: D101480: Revert "[Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable".
Wed, Apr 28, 12:12 PM · Restricted Project
mbenfield added a comment to D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable.

Also, from the reviewer list, it doesn't look like anyone with much familiarity with clang/clang diags actually participated in the review?
I think this might be the point to revert and re-review.

Wed, Apr 28, 11:16 AM · Restricted Project
mbenfield added a comment to D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable.

One more false-positive:

void foo() {
  extern int yydebug;
  yydebug = 1;
}

It was triggered in ISPC build.

Wed, Apr 28, 10:18 AM · Restricted Project

Mon, Apr 26

mbenfield added a comment to D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable.

I can land this for you, but could you update the description to be a bit more descriptive? Explaining what the warnings do.

Mon, Apr 26, 1:51 PM · Restricted Project

Fri, Apr 23

mbenfield added a comment to D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable.

Great. If it's been approved, I am not able to commit, so I think someone else will have to do that.

Fri, Apr 23, 10:50 AM · Restricted Project

Thu, Apr 22

mbenfield updated the diff for D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable.

Fix test warning-wall.c.

Thu, Apr 22, 2:08 PM · Restricted Project
mbenfield added a comment to D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable.

Meanwhile I collected some interesting testcases from gcc bugzilla (reported as false positives), please try:
https://godbolt.org/z/747ndKEvd - No "unused-but-set" warnings expected.

Thu, Apr 22, 2:06 PM · Restricted Project

Wed, Apr 21

mbenfield added a comment to D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable.

So I think we should put UnusedButSetParameter under -Wextra

So I would just put UnusedButSetVariable under -Wunused (and -Wunused is part of -Wall):

WDYT?

Wed, Apr 21, 9:39 AM · Restricted Project
mbenfield updated the diff for D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable.

Response to review comments.

Wed, Apr 21, 9:38 AM · Restricted Project

Mon, Apr 19

mbenfield added a comment to D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable.

I am a little bit worried that another off by default warning is not ideal from user point of view. Either the user simply would fail to find out that there is a new option or will be surprised that gcc fires and clang does not even when we claim we implemented this “gcc’s” warning.

Mon, Apr 19, 5:02 PM · Restricted Project

Sun, Apr 18

mbenfield updated the diff for D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable.

Fixed misfirings reported by aeubanks and added tests for those cases.

Sun, Apr 18, 8:49 PM · Restricted Project
mbenfield added a comment to D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable.

These warnings are not enabled by any other flags. This is different from gcc, where -Wunused-but-set-variable is enabled by -Wextra in combination with either -Wunused or -Wall.

IMHO we should follow gcc here.

Sun, Apr 18, 8:44 PM · Restricted Project

Apr 16 2021

mbenfield added inline comments to D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable.
Apr 16 2021, 3:18 PM · Restricted Project
mbenfield added inline comments to D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable.
Apr 16 2021, 8:24 AM · Restricted Project

Apr 15 2021

mbenfield added inline comments to D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable.
Apr 15 2021, 11:34 PM · Restricted Project
mbenfield updated the diff for D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable.

Capitalization and periods for comments.

Apr 15 2021, 11:18 PM · Restricted Project
mbenfield updated the diff for D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable.

Respond to comments.

Apr 15 2021, 11:14 PM · Restricted Project
mbenfield added inline comments to D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable.
Apr 15 2021, 3:21 PM · Restricted Project
mbenfield updated the diff for D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable.

Updates in response to comments.

Apr 15 2021, 12:48 PM · Restricted Project
mbenfield added a comment to D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable.

running this over an existing codebase to see what fires is probably a good idea (if you haven't already done that)

Apr 15 2021, 12:29 PM · Restricted Project
mbenfield added inline comments to D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable.
Apr 15 2021, 11:49 AM · Restricted Project
mbenfield updated the diff for D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable.

bugfix: don't increment FalseCount when the value was already false

Apr 15 2021, 11:20 AM · Restricted Project
mbenfield added a comment to D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable.

This is my first code submitted to LLVM; please tell me anything that should be done differently.

Apr 15 2021, 11:01 AM · Restricted Project
mbenfield requested review of D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable.
Apr 15 2021, 10:54 AM · Restricted Project