[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

xbolva00 added a comment. In D100581#3404624 , @xbolva00 wrote: > @mbenfield found false positive > > void test(void) { > static int counter = 0; > counter += 5; > } posted https://reviews.llvm.org/D122374 Repository: rG LLVM Github

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

xbolva00 added a comment. Herald added subscribers: pcwang-thead, luke957. Herald added a project: All. @mbenfield found false positive void test(void) { static int counter = 0; counter += 5; } Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

Abpostelnicu added a comment. In D100581#2798079 , @raj.khem wrote: > http://sprunge.us/FJzZXL is a file from harfbuzz and it warns > > a.cc:28670:32: error: variable 'supp_size' set but not used > [-Werror,-Wunused-but-set-variable] > unsigned

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

xbolva00 added a comment. In D100581#2798079 , @raj.khem wrote: > http://sprunge.us/FJzZXL is a file from harfbuzz and it warns > > a.cc:28670:32: error: variable 'supp_size' set but not used > [-Werror,-Wunused-but-set-variable] > unsigned int

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

raj.khem added a comment. http://sprunge.us/FJzZXL is a file from harfbuzz and it warns a.cc:28670:32: error: variable 'supp_size' set but not used [-Werror,-Wunused-but-set-variable] unsigned int size0, size1, supp_size = 0; I do not have -Werror enabled but it still is reported as

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

mbenfield added a comment. In D100581#2795966 , @sberg wrote: > Is it intentional that this warns about volatile variables as in Yes, volatile was intentional. Alright, I will add a test for this case. Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

sberg added a comment. Is it intentional that this warns about volatile variables as in void f(char * p) { volatile char c = 0; c ^= *p; } (I see that GCC warns about volatile too, at least when you replace the `^=` with `=`, so assume the answer is "yes", but would just like

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

mbenfield added a comment. In D100581#2792854 , @Abpostelnicu wrote: > I think there is a false positive with this @george.burgess.iv: > In this >

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

ldionne added a comment. In D100581#2793926 , @dblaikie wrote: > In D100581#2793775 , @ldionne wrote: > >> Hello! There are still some false positives, for example this one is >> breaking the libc++ CI: >>

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

dblaikie added a comment. In D100581#2793775 , @ldionne wrote: > Hello! There are still some false positives, for example this one is breaking > the libc++ CI: >

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

ldionne added a comment. Hello! There are still some false positives, for example this one is breaking the libc++ CI: https://buildkite.com/llvm-project/libcxx-ci/builds/3530#8679608a-200b-4a48-beb4-a9e9924a8848 Would it be possible to either fix this quickly or revert the change until the

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

xbolva00 added a comment. In D100581#2792854 , @Abpostelnicu wrote: > I think there is a false positive with this @george.burgess.iv: > In this >

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

Abpostelnicu added a comment. I think there is a false positive with this @george.burgess.iv: In [this)(https://searchfox.org/mozilla-central/source/mozglue/baseprofiler/core/platform-linux-android.cpp#222-227) we have the warning triggered:

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

This revision was automatically updated to reflect the committed changes. Closed by commit rGcf49cae278b4: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable (authored by mbenfield, committed by george.burgess.iv). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

lebedev.ri resigned from this revision. lebedev.ri added a comment. This revision is now accepted and ready to land. Removing from queue - i don't expect to review this. Looks like this has been reverted twice now, presumably llvm stage 2/linux kernel/chrome should be enough of a coverage to be

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

mbenfield updated this revision to Diff 347111. mbenfield added a comment. Move fixes into a separate change https://reviews.llvm.org/D102942. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100581/new/ https://reviews.llvm.org/D100581 Files:

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

xbolva00 added a comment. >> Also fix a few places where this warning is correctly triggered. Create new patch please - dont mix fixes with new warning within one patch. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100581/new/

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

mbenfield updated this revision to Diff 346904. mbenfield added a comment. Herald added subscribers: llvm-commits, lldb-commits, kbarton, hiraditya, nemanjai. Herald added projects: LLDB, LLVM. Don't warn for reference or dependent types (fixing false positives). Also fix a few places where

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

pcc added a comment. This warning seems to have a lot of false positives on things like reference arguments that are used as output parameters. For example here is a small sample of output from a stage2 build of part of LLVM: In file included from ../llvm/lib/BinaryFormat/Minidump.cpp:9:

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit rG14dfb3831c42: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable (authored by mbenfield, committed by

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

aeubanks accepted this revision. aeubanks added a comment. If you'd like you can split this into separate changes, one for adding the warning and another for adding it into warning groups, either way is fine. Seems like the reported false positives have been addressed. Repository: rG LLVM

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

mbenfield added a comment. FWIW `gcc` does not warn in this case regardless of any cast: void f() { int x; x = 0; sizeof(x); } Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100581/new/ https://reviews.llvm.org/D100581

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

aeubanks added a comment. casting to void is the normal way to get around these warnings, these new warnings should also not diagnose anything with a cast to void Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100581/new/

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

dblaikie added a comment. In D100581#2730743 , @nickdesaulniers wrote: > Testing Diff 342071 on the mainline Linux kernel, just building x86_64 > defconfig triggers 19 instances of this warning; all look legit. ;) > > In D100581#2730708

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

nickdesaulniers added a comment. In D100581#2730760 , @mbenfield wrote: > In D100581#2730557 , > @nickdesaulniers wrote: > >> I see lots of instances from the kernel that look like this when reduced: > > > >>

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

mbenfield added a comment. In D100581#2730557 , @nickdesaulniers wrote: > I see lots of instances from the kernel that look like this when reduced: > But adding a new warning flags to a handful of existing tests' RUN lines is > unusual. These tests

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

nickdesaulniers added a comment. Testing Diff 342071 on the mainline Linux kernel, just building x86_64 defconfig triggers 19 instances of this warning; all look legit. ;) In D100581#2730708 , @dblaikie wrote: > In D100581#2730557

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

nickdesaulniers added a comment. The additions of `-Wno-unused-but-set-variable` to: - clang/test/SemaObjC/foreach.m - clang/test/SemaCXX/sizeless-1.cpp - clang/test/SemaCXX/shift.cpp - clang/test/SemaCXX/goto.cpp - clang/test/Sema/vector-gcc-compat.cpp - clang/test/Sema/vector-gcc-compat.c -

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

dblaikie added a comment. In D100581#2730557 , @nickdesaulniers wrote: > I see lots of instances from the kernel that look like this when reduced: > > $ cat foo.c > int page; > int put_page_testzero(int); > void foo (void) { > int zeroed; >

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

mbenfield updated this revision to Diff 342071. mbenfield added a comment. Count a non-ODR use as a reference, so that examples like that of nickdesaulniers don't warn. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100581/new/

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

nickdesaulniers added a comment. I see lots of instances from the kernel that look like this when reduced: $ cat foo.c int page; int put_page_testzero(int); void foo (void) { int zeroed; zeroed = put_page_testzero(page); ((void)(sizeof(( long)(!zeroed; } $ clang -c

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

mbenfield updated this revision to Diff 342037. mbenfield added a comment. Correct struct and vector behavior. gcc doesn't warn for any structs in C++. However, it _does_ warn for vector types. Those warnings are sometimes masked by other errors, leading to my earlier belief that it is

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

mbenfield added a comment. 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100581/new/

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

nickdesaulniers added inline comments. Comment at: clang/include/clang/Basic/DiagnosticGroups.td:876 UnusedLocalTypedef, UnusedValue, UnusedVariable, -UnusedPropertyIvar]>, +UnusedButSetVariable,

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

mbenfield updated this revision to Diff 341965. mbenfield added a comment. A second commit with UnusedButSetParameter and UnusedButSetVariable into groups. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100581/new/ https://reviews.llvm.org/D100581

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

dblaikie added a comment. In D100581#2727432 , @mbenfield wrote: > In D100581#2727357 , @dblaikie > wrote: > >> > > > >> Got a link/examples of cases GCC does and doesn't warn about? I'd assume >> it'd have

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

xbolva00 added a comment. In D100581#2727460 , @aeubanks wrote: > regarding adding these to a warning group in a separate change, it's easier > to revert a smaller second change that adds this to warning groups, and > easier to iterate upon in tree

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

aeubanks added a comment. regarding adding these to a warning group in a separate change, it's easier to revert a smaller second change that adds this to warning groups, and easier to iterate upon in tree when there are discovered issues it's not dead code if it has tests Repository: rG

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

mbenfield added a comment. Will also make a new revision tomorrow with these warnings added to the appropriate groups. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100581/new/ https://reviews.llvm.org/D100581

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

mbenfield added a comment. In D100581#2727357 , @dblaikie wrote: > > Got a link/examples of cases GCC does and doesn't warn about? I'd assume it'd > have something to do with the triviality or non-triviality of certain > operations of the nonscalar

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

nickdesaulniers added a comment. I'm happy to retest with a few Linux kernel builds...tomorrow! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100581/new/ https://reviews.llvm.org/D100581 ___ cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

xbolva00 added a comment. >> I've removed these warnings from the diagnostic groups -Wextra But to remove all false positives, you need reports from users of -Wextra. Otherwise this would be dead code with no users. I suggest to leave it in groups. Now you can ask folks to try this patch with

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

dblaikie added a comment. In D100581#2727330 , @mbenfield wrote: > Another try at these warnings, using the implementation strategy outlined by > rsmith. > > A couple other notes: > > - At the moment I've removed these warnings from the diagnostic

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

mbenfield updated this revision to Diff 341695. mbenfield added a comment. Another try at these warnings, using the implementation strategy outlined by rsmith. A couple other notes: - At the moment I've removed these warnings from the diagnostic groups -Wextra and -Wunused. It was suggested

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

mbenfield added a comment. In D100581#2723801 , @xbolva00 wrote: > > Yes, the best solution. Also consider to check gcc’s test-suite - if they > have this type of testcase, then this is wanted behaviour. gcc has `Wunused-var-5.c`, where the warning

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

rsmith added a comment. When this change returns, I'd like to see a different implementation strategy. Running a recursive AST visitation after the fact is generally not the right way to look for this kind of issue; adding an extra pass to speculatively hunt for each kind of warning we might

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

xbolva00 added a comment. In D100581#2723794 , @nickdesaulniers wrote: > In D100581#2723789 , @mbenfield > wrote: > >> But gcc's behavior is to not trigger here so maybe following it is best. > > Perhaps file a

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

nickdesaulniers added a comment. In D100581#2723789 , @mbenfield wrote: > But gcc's behavior is to not trigger here so maybe following it is best. Perhaps file a bug against GCC to find whether that was intentional or a bug? Repository: rG LLVM

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

mbenfield added a comment. So it seems there are 3 issues here: 1. local variables declared extern should not trigger this warning. I'll fix this. 2. Should `x` in this code trigger the warning: int x; int y = (x = 0); These are essentially the "false positives" reported in the Linux

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

lebedev.ri added a comment. In D100581#2723709 , @aeubanks wrote: > reverted > regarding having a reviewer who is knowledgable in clang diagnostics, I > assumed that george.burgess.iv was knowledgable and was happy with the change > after his comments,

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

aeubanks added a comment. reverted regarding having a reviewer who is knowledgable in clang diagnostics, I assumed that george.burgess.iv was knowledgable and was happy with the change after his comments, perhaps I should have waited for an explicit LGTM Repository: rG LLVM Github Monorepo

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

lebedev.ri added a comment. In D100581#2723621 , @Abpostelnicu wrote: > In D100581#2723611 , @lebedev.ri > wrote: > >> In D100581#2723505 , @Abpostelnicu >> wrote: >>

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

Abpostelnicu added a comment. In D100581#2723611 , @lebedev.ri wrote: > In D100581#2723505 , @Abpostelnicu > wrote: > >> Also I don’t remember seeing this proposed on cfe dev mailing list. > > There is no such

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

lebedev.ri added a comment. In D100581#2723505 , @Abpostelnicu wrote: > Also I don’t remember seeing this proposed on cfe dev mailing list. There is no such requirement. I don't recall that happening basically ever, actually. Repository: rG LLVM

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

mbenfield added a comment. Link for the revert: https://reviews.llvm.org/D101480 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100581/new/ https://reviews.llvm.org/D100581 ___ cfe-commits mailing list

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

Abpostelnicu added a comment. Please revert this. This change hasn’t been tested nor reviewed enough. Also I don’t remember seeing this proposed on cfe dev mailing list. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100581/new/

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

mbenfield added a comment. In D100581#2723348 , @lebedev.ri wrote: > 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

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

lebedev.ri added a comment. 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

lebedev.ri added a comment. In D100581#2723329 , @nathanchance wrote: > In D100581#2721430 , > @nickdesaulniers wrote: > >> Huh, maybe not: https://godbolt.org/z/PnE1fMGWo > > This difference has caused some

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

nathanchance added a comment. In D100581#2721430 , @nickdesaulniers wrote: > Huh, maybe not: https://godbolt.org/z/PnE1fMGWo This difference has caused some confusion already for the Linux kernel:

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

dbabokin added a comment. > Thanks for the report. It seems the thing to do is to modify the warning so > that variables marked extern are not candidates for this warning. I will make > a new commit with that modification unless there are other suggestions. I agree, this seems to be the right

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

mbenfield added a comment. In D100581#2721705 , @dbabokin wrote: > One more false-positive: > > void foo() { > extern int yydebug; > yydebug = 1; > } > > It was triggered in ISPC build. Thanks for the report. It seems the thing to do is to

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

dbabokin added a comment. One more false-positive: void foo() { extern int yydebug; yydebug = 1; } It was triggered in ISPC build. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100581/new/ https://reviews.llvm.org/D100581

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

nickdesaulniers added a comment. Huh, maybe not: https://godbolt.org/z/PnE1fMGWo Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100581/new/ https://reviews.llvm.org/D100581 ___ cfe-commits mailing list

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

nickdesaulniers added a comment. I think this is triggering some false positives in builds of the Linux kernel. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100581/new/ https://reviews.llvm.org/D100581

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

eugenis added a comment. Right, our commit process is a bit strange. Ideally, the change author should initiate commit after they have the necessary approvals, and then be responsible to deal with any fallout in a timely manner. As things are, the author does not control the timing of the

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

aeubanks added a comment. Sorry about that, although I don't think emails went to me since the email was somebody else's. But point taken, I'll coordinate better in the future when landing someone else's patch. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

eugenis added a comment. I fixed another one in https://reviews.llvm.org/rG561f4b9087457e9ea3cf4aeb57dcd507e2fa6258 Please watch the bots after you land changes like this one and don't let them stay broken for an entire day! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

aeubanks added a comment. In D100581#2718947 , @Abpostelnicu wrote: > I think this added a regression, where you have this context: > > uint32_t LocalAccessible::SelectedItemCount() { > uint32_t count = 0; > AccIterator iter(this,

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

aeubanks added a comment. In D100581#2718840 , @uabelho wrote: > I noticed that with this patch > > libcxx/test/std/strings/basic.string/string.cons/iter_alloc_deduction.fail.cpp > fails: > > error: 'warning' diagnostics seen but not expected: >

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

Abpostelnicu added a comment. I think this added a regression, where you have this context: uint32_t LocalAccessible::SelectedItemCount() { uint32_t count = 0; AccIterator iter(this, filters::GetSelected); LocalAccessible* selected = nullptr; while ((selected = iter.Next()))

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

uabelho added a comment. I noticed that with this patch libcxx/test/std/strings/basic.string/string.cons/iter_alloc_deduction.fail.cpp fails: error: 'warning' diagnostics seen but not expected: File

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

This revision was automatically updated to reflect the committed changes. Closed by commit rG9b0501abc7b5: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable (authored by mbenfield, committed by aeubanks). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

aeubanks added a comment. yeah you have to manually go to "Edit Revision" and update the message. I'll just amend it locally though Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100581/new/ https://reviews.llvm.org/D100581

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

mbenfield added a comment. In D100581#2717182 , @aeubanks wrote: > I can land this for you, but could you update the description to be a bit > more descriptive? Explaining what the warnings do. Are you looking at the updated message in the commit, or

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

aeubanks added a comment. I can land this for you, but could you update the description to be a bit more descriptive? Explaining what the warnings do. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100581/new/ https://reviews.llvm.org/D100581

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

mbenfield added a comment. Great. If it's been approved, I am not able to commit, so I think someone else will have to do that. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100581/new/ https://reviews.llvm.org/D100581

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

xbolva00 added a comment. I think this is good to go. Later anybody can build improvements on top of this patch and maybe work on some more powerful “dead stores” warnings. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100581/new/

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

aeubanks added a comment. Looks good in terms of false positives on Chrome. This did uncover a lot of issues, although many of them are due to only using some variable in some config with `#ifdef` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

mbenfield updated this revision to Diff 339778. mbenfield added a comment. Fix test warning-wall.c. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100581/new/ https://reviews.llvm.org/D100581 Files: clang/include/clang/Basic/DiagnosticGroups.td

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

mbenfield marked 4 inline comments as done. mbenfield added a comment. In D100581#2705780 , @xbolva00 wrote: > Meanwhile I collected some interesting testcases from gcc bugzilla (reported > as false positives), please try: >

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

dblaikie added a comment. In D100581#2706554 , @xbolva00 wrote: > In D100581#2706467 , @dblaikie > wrote: > >> FWIW, I'd love it if we could do a full dead-store warning, which would be a >> superset of this. I

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

xbolva00 added a comment. In D100581#2706467 , @dblaikie wrote: > FWIW, I'd love it if we could do a full dead-store warning, which would be a > superset of this. I think we have enough infrastructure in the analysis based > warnings (I think the

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

dblaikie added a comment. FWIW, I'd love it if we could do a full dead-store warning, which would be a superset of this. I think we have enough infrastructure in the analysis based warnings (I think the sufficiency of the infrastructure is demonstrated by the "may be used uninitialized"

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

xbolva00 added a comment. Thanks, looks really good. 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

mbenfield added a comment. In D100581#2700083 , @xbolva00 wrote: > So I think we should put UnusedButSetParameter under -Wextra > > So I would just put UnusedButSetVariable under -Wunused (and -Wunused is part > of -Wall): > > WDYT? Sounds good; I've

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

mbenfield updated this revision to Diff 339275. mbenfield added a comment. Herald added subscribers: frasercrmck, luismarques, apazos, sameer.abuasal, pengfei, s.egerton, Jim, jocewei, PkmX, the_o, brucehoult, MartinMosbeck, rogfer01, edward-jones, zzheng, jrtc27, niosHD, sabuasal, simoncook,

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

xbolva00 added a comment. So I checked gcc on godbolt since gcc docs are not so clear. UnusedButSetParameter is ignored with: - -Wunused alone - -Wextra alone - -Wall -Wunused But works with -Wextra -Wunused or -Wall -Wextra. So I think we should put UnusedButSetParameter under -Wextra

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

mbenfield added a comment. In D100581#2699412 , @xbolva00 wrote: > 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

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

aeubanks added a comment. another false positive: ../../third_party/abseil-cpp/absl/strings/internal/str_format/float_conversion.cc:879:20: error: variable 'kBufSizeForHexFloatRepr' set but not used [-Werror,-Wunused-but-set-variable] constexpr size_t kBufSizeForHexFloatRepr =

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

xbolva00 added a subscriber: rsmith. xbolva00 added a comment. In D100581#2697697 , @mbenfield wrote: > In D100581#2693131 , @xbolva00 > wrote: > These warnings are not enabled by any other flags. This is

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

george.burgess.iv added a reviewer: rtrieu. george.burgess.iv added a comment. Just a few more nits and LGTM. We probably want the thoughts of someone with ownership in warnings to be sure. +rtrieu might be good? Comment at: clang/lib/Sema/SemaDecl.cpp:13740 +// other than

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

mbenfield updated this revision to Diff 338423. mbenfield added a comment. Fixed misfirings reported by aeubanks and added tests for those cases. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100581/new/ https://reviews.llvm.org/D100581 Files:

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

mbenfield added a comment. In D100581#2693131 , @xbolva00 wrote: >>> 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

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

mbenfield added inline comments. Comment at: clang/lib/Sema/SemaStmt.cpp:437 + CompoundStmt *CS = CompoundStmt::Create(Context, Elts, L, R); + DiagnoseUnusedButSetVariables(CS); + return CS; mbenfield wrote: > mbenfield wrote: > > mbenfield wrote: > > >

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

aeubanks added a comment. -Wunused-but-set-variable is firing on `x` at [1] [1] https://source.chromium.org/chromium/chromium/src/+/master:third_party/abseil-cpp/absl/synchronization/mutex.h;drc=07cb7b184515ad207d30f00d0b00b8ce96d0a750;l=947

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

aeubanks added a comment. Running this over Chromium, I'm getting $ cat /tmp/a.cc struct A { int i; A(int i): i(i) {} }; $ ./build/rel/bin/clang++ -fsyntax-only /tmp/a.cc -Wunused-but-set-parameter /tmp/a.cc:3:9: warning: parameter 'i' set but not used

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

mbenfield added inline comments. Comment at: clang/lib/Sema/SemaStmt.cpp:437 + CompoundStmt *CS = CompoundStmt::Create(Context, Elts, L, R); + DiagnoseUnusedButSetVariables(CS); + return CS; mbenfield wrote: > mbenfield wrote: > > george.burgess.iv wrote: > >

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

mbenfield marked 4 inline comments as done. mbenfield added inline comments. Comment at: clang/lib/Sema/SemaDecl.cpp:13751 +bool TraverseBinaryOperator(BinaryOperator *BO) { + auto *LHS = BO->getLHS(); + auto *DRE = dyn_cast(LHS); george.burgess.iv

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

mbenfield updated this revision to Diff 337999. mbenfield marked an inline comment as done. mbenfield added a comment. Capitalization and periods for comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100581/new/

  1   2   >