Page MenuHomePhabricator

logan-5 (Logan Smith)
User

Projects

User does not belong to any projects.

User Details

User Since
Jul 1 2019, 8:51 AM (58 w, 2 d)

Recent Activity

Mon, Jul 27

logan-5 committed rGa52aea0ba624: Use INTERFACE_COMPILE_OPTIONS to disable -Wsuggest-override for any target that… (authored by logan-5).
Use INTERFACE_COMPILE_OPTIONS to disable -Wsuggest-override for any target that…
Mon, Jul 27, 8:38 AM
logan-5 closed D84554: Use INTERFACE_COMPILE_OPTIONS to disable -Wsuggest-override for any target that links to gtest.
Mon, Jul 27, 8:38 AM · Restricted Project, Restricted Project, Restricted Project
logan-5 added a comment to D84554: Use INTERFACE_COMPILE_OPTIONS to disable -Wsuggest-override for any target that links to gtest.

Could you elaborate on the lldb issue? I'd like to take a look at that...

Mon, Jul 27, 8:32 AM · Restricted Project, Restricted Project, Restricted Project

Fri, Jul 24

Herald added projects to D84554: Use INTERFACE_COMPILE_OPTIONS to disable -Wsuggest-override for any target that links to gtest: Restricted Project, Restricted Project, Restricted Project.
Fri, Jul 24, 1:23 PM · Restricted Project, Restricted Project, Restricted Project
logan-5 added a comment to D84126: Enable -Wsuggest-override in the LLVM build.

Instead of adding -Wno-suggest-override to every user it might be cleaner to add the flag to the INTERFACE_COMPILE_OPTIONS property of the relevant targets (gtest, gmock, and gbenchmark ?). That way, anything which links against these will inherit it automatically.

Fri, Jul 24, 10:23 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project

Thu, Jul 23

logan-5 added a comment to D84244: [clang] Disable -Wsuggest-override for unittests/.

Looks good to me so far. We haven't tried doing any full builds yet, but I checked for the specific problem I hit yesterday (building ClangApplyReplacementsTests) and that's working fine now.

Thu, Jul 23, 8:51 AM · Restricted Project

Wed, Jul 22

logan-5 committed rG77e0e9e17daf: Reapply "Try enabling -Wsuggest-override again, using add_compile_options… (authored by logan-5).
Reapply "Try enabling -Wsuggest-override again, using add_compile_options…
Wed, Jul 22, 5:50 PM
logan-5 committed rG97a0f80c469c: Revert "Try enabling -Wsuggest-override again, using add_compile_options… (authored by logan-5).
Revert "Try enabling -Wsuggest-override again, using add_compile_options…
Wed, Jul 22, 3:07 PM
logan-5 added a reverting change for rG388c9fb1af48: Try enabling -Wsuggest-override again, using add_compile_options instead of…: rG97a0f80c469c: Revert "Try enabling -Wsuggest-override again, using add_compile_options….
Wed, Jul 22, 3:07 PM
logan-5 added a comment to D84244: [clang] Disable -Wsuggest-override for unittests/.

Sometimes it's OK to commit stuff an see how it goes on the buildbots - doing so out of peak times and with the intention of rolling back quickly/immediately if the buildbots report breakage is sometimes the best tool we have (not great, but such is the way of things).

In that case, I've enabled it again using add_compile_options instead of add_definitions. I've got my finger hovering over the button to revert.

Wed, Jul 22, 2:22 PM · Restricted Project
logan-5 committed rG388c9fb1af48: Try enabling -Wsuggest-override again, using add_compile_options instead of… (authored by logan-5).
Try enabling -Wsuggest-override again, using add_compile_options instead of…
Wed, Jul 22, 2:20 PM
logan-5 added a comment to D84244: [clang] Disable -Wsuggest-override for unittests/.

I don't really know why this doesn't happen with other warning flags, but I think it would be better to add flags like this with add_compile_options rather than add_compile_definitions. I imagine that would prevent it from reaching rc.exe.

That sounds pretty reasonable. Without any way of testing the Windows setup myself though, I don't feel comfortable making that change.

Wed, Jul 22, 12:24 PM · Restricted Project
logan-5 added a comment to D84244: [clang] Disable -Wsuggest-override for unittests/.

Thanks for reverting--I agree that that's the right move at this point.

Wed, Jul 22, 12:00 PM · Restricted Project
logan-5 committed rG1c7037a2a557: [clangd] Disable -Wsuggest-override for unittests/ (authored by logan-5).
[clangd] Disable -Wsuggest-override for unittests/
Wed, Jul 22, 10:49 AM
logan-5 committed rG274b6b0c7a8b: Only enable -Wsuggest-override if it doesn't suggest adding override to… (authored by logan-5).
Only enable -Wsuggest-override if it doesn't suggest adding override to…
Wed, Jul 22, 10:06 AM
logan-5 closed D84292: Only enable -Wsuggest-override if it doesn't suggest adding override to functions that are already final.
Wed, Jul 22, 10:06 AM · Restricted Project
logan-5 added a comment to D84126: Enable -Wsuggest-override in the LLVM build.

I'd suggest disabling this unless the GCC version is >9.1.

I've got a patch addressing exactly this issue here, hoping to get it landed soon: https://reviews.llvm.org/D84292

Wed, Jul 22, 9:55 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project

Tue, Jul 21

Herald added a project to D84292: Only enable -Wsuggest-override if it doesn't suggest adding override to functions that are already final: Restricted Project.
Tue, Jul 21, 9:36 PM · Restricted Project
logan-5 added a comment to D84126: Enable -Wsuggest-override in the LLVM build.

All the failures related to this patch are now resolved as far as I can tell. I'm very sorry for the trouble.

Tue, Jul 21, 7:01 PM · Restricted Project, Restricted Project, Restricted Project, Restricted Project
logan-5 committed rG21c0b4c1e8d6: Disable -Wsuggest-override for all remaining unittests/ directories (authored by logan-5).
Disable -Wsuggest-override for all remaining unittests/ directories
Tue, Jul 21, 5:49 PM
logan-5 committed rG81d68ad27b29: [lld] Disable -Wsuggest-override for unittests (authored by logan-5).
[lld] Disable -Wsuggest-override for unittests
Tue, Jul 21, 5:41 PM
logan-5 added a comment to D84244: [clang] Disable -Wsuggest-override for unittests/.

Looks OK (for future reference, this sort of stuff should've been cleaned up before enabling the flag so as to avoid this kind of hurry/breakage/etc)

Tue, Jul 21, 4:48 PM · Restricted Project
logan-5 committed rGa361aa524985: [clang] Disable -Wsuggest-override for unittests/ (authored by logan-5).
[clang] Disable -Wsuggest-override for unittests/
Tue, Jul 21, 4:39 PM
logan-5 added a comment to D84244: [clang] Disable -Wsuggest-override for unittests/.

Committing this now to fix the bots, as per discussion in https://reviews.llvm.org/D84126. I'll happily revert and approach it differently later if anyone requests that.

Tue, Jul 21, 4:37 PM · Restricted Project
logan-5 added a comment to D84126: Enable -Wsuggest-override in the LLVM build.

There is a lot of instances where this warning fires in the unit tests since gtest does not appear to use the override keyword. This in turn causes failures on bootstrap builds that use -Werror.
An example: http://lab.llvm.org:8011/builders/ppc64le-lld-multistage-test/builds/10717

Can we either get this pulled until the failures in bootstrap are fixed or quickly fix the code that causes the failures?

Tue, Jul 21, 4:36 PM · Restricted Project, Restricted Project, Restricted Project, Restricted Project
logan-5 added a comment to D84244: [clang] Disable -Wsuggest-override for unittests/.

For GTest and GMock specifically, it would be a good medium-term idea to try to upstream patches into those libraries. I did eventually have success upstreaming fixes for -Wdeprecated into GTest, for example: https://github.com/google/googletest/pull/2815

Nice, that's encouraging. That does sound like a good solution for slightly further down the road.

(Although I'm a bit confused; I thought -Wsuggest-override was off-by-default? Is it part of -Wall, and do unittests add -Wall to their command line or something?)

After I implemented the warning and it landed, people wanted it explicitly turned on in the LLVM build, so then I did that. This is part of the fallout from that.

Tue, Jul 21, 9:43 AM · Restricted Project
logan-5 committed rGfa42b7cf2949: [clang-tools-extra] Disable -Wsuggest-override for unittests/ (authored by logan-5).
[clang-tools-extra] Disable -Wsuggest-override for unittests/
Tue, Jul 21, 9:13 AM
logan-5 closed D84213: [clang-tools-extra] Disable -Wsuggest-override for unittests/.
Tue, Jul 21, 9:13 AM · Restricted Project, Restricted Project
logan-5 created D84244: [clang] Disable -Wsuggest-override for unittests/.
Tue, Jul 21, 9:10 AM · Restricted Project

Mon, Jul 20

logan-5 committed rGa58a8c017015: [NFC] Add another missing 'override' (authored by logan-5).
[NFC] Add another missing 'override'
Mon, Jul 20, 10:05 PM
logan-5 committed rG865ee64bf80c: [NFC] Add missing 'override's (authored by logan-5).
[NFC] Add missing 'override's
Mon, Jul 20, 9:19 PM
logan-5 committed rG955f87f947fd: [compiler-rt] Disable -Wsuggest-override for unittests (authored by logan-5).
[compiler-rt] Disable -Wsuggest-override for unittests
Mon, Jul 20, 9:19 PM
logan-5 committed rGfc24d1eaddd8: [clang][NFC] Add missing 'override's (authored by logan-5).
[clang][NFC] Add missing 'override's
Mon, Jul 20, 9:19 PM
logan-5 committed rG8ed021382e6b: Fix typo causing build failure (authored by logan-5).
Fix typo causing build failure
Mon, Jul 20, 9:19 PM
logan-5 committed rG8b6179f48c6c: [NFC] Add missing 'override's (authored by logan-5).
[NFC] Add missing 'override's
Mon, Jul 20, 9:18 PM
logan-5 committed rG308a127a38d1: [llvm][unittest] Add -Wno-suggest-override to more infrastructure that includes… (authored by logan-5).
[llvm][unittest] Add -Wno-suggest-override to more infrastructure that includes…
Mon, Jul 20, 9:18 PM
logan-5 committed rG8b16e45f66e2: Enable -Wsuggest-override in the LLVM build (authored by logan-5).
Enable -Wsuggest-override in the LLVM build
Mon, Jul 20, 9:18 PM
logan-5 closed D84126: Enable -Wsuggest-override in the LLVM build.
Mon, Jul 20, 9:17 PM · Restricted Project, Restricted Project, Restricted Project, Restricted Project
Herald added a project to D84213: [clang-tools-extra] Disable -Wsuggest-override for unittests/: Restricted Project.
Mon, Jul 20, 9:16 PM · Restricted Project, Restricted Project

Sun, Jul 19

logan-5 added a comment to D82728: [clang] Add -Wsuggest-override.

Is it possible to emit fixit note with "override" ?

This is a good idea, though unfortunately (after eyeballing the implementation of modernize-use-override in clang-tidy (UseOverrideCheck.cpp)), it looks non-trivial to figure out where exactly to insert override. There's some significant logic in the clang-tidy check involving re-lexing the relevant tokens, to find the insertion point in the presence of complexity like inline definitions, = 0, = {delete|default}, function try blocks, macros, and the list goes on.

Sun, Jul 19, 4:43 PM · Restricted Project
Herald added a reviewer for D84126: Enable -Wsuggest-override in the LLVM build: Restricted Project.
Sun, Jul 19, 9:57 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project
logan-5 committed rG19dd3712e5ae: [llvm][NFC] Add missing 'override' (authored by logan-5).
[llvm][NFC] Add missing 'override'
Sun, Jul 19, 9:57 AM
logan-5 added a comment to D72282: [clang-tidy] Add `bugprone-unintended-adl`.

Pinging this. I believe all feedback from @EricWF was addressed back in my update to the patch in March.

Sun, Jul 19, 9:41 AM · Restricted Project, Restricted Project

Fri, Jul 17

logan-5 committed rG105056045d9a: [clang][NFC] Add a missing 'override' (authored by logan-5).
[clang][NFC] Add a missing 'override'
Fri, Jul 17, 5:37 PM
logan-5 committed rG3ee7fe4cfda1: [llvm][NFC] Add missing 'override's (authored by logan-5).
[llvm][NFC] Add missing 'override's
Fri, Jul 17, 5:37 PM
logan-5 committed rG31eb83496fb4: [llvm][NFC] Add missing 'override's in unittests/ (authored by logan-5).
[llvm][NFC] Add missing 'override's in unittests/
Fri, Jul 17, 5:37 PM

Thu, Jul 16

logan-5 committed rG5d31d09f768a: [polly][NFC] Add missing 'override's (authored by logan-5).
[polly][NFC] Add missing 'override's
Thu, Jul 16, 8:12 PM
logan-5 committed rG947bf0fdf6e6: [compiler-rt][NFC] Add missing 'override's (authored by logan-5).
[compiler-rt][NFC] Add missing 'override's
Thu, Jul 16, 8:12 PM

Wed, Jul 15

logan-5 committed rG44b43a52dc17: [lldb][NFC] Add 'override' where missing in source/ and tools/ (authored by logan-5).
[lldb][NFC] Add 'override' where missing in source/ and tools/
Wed, Jul 15, 11:35 AM
logan-5 closed D83847: [lldb][NFC] Add 'override' where missing in source/ and tools/.
Wed, Jul 15, 11:35 AM · Restricted Project

Tue, Jul 14

logan-5 created D83847: [lldb][NFC] Add 'override' where missing in source/ and tools/.
Tue, Jul 14, 10:13 PM · Restricted Project
logan-5 added a comment to D83611: [clang][NFC] Add 'override' keyword to virtual function overrides.

If you add/leave the "Differential Revision: https://reviews.llvm.org/DXXXX" line in the commit message (arc will add this line automatically) Phabricator will close the review for you automatically

Thanks--figured that out shortly after I did this one. :)

Tue, Jul 14, 1:27 PM · Restricted Project
logan-5 closed D83611: [clang][NFC] Add 'override' keyword to virtual function overrides.

Committed as rG2c2a297bb6d1

Tue, Jul 14, 11:59 AM · Restricted Project
logan-5 committed rGa19461d9e114: [NFC] Add 'override' keyword where missing in include/ and lib/. (authored by logan-5).
[NFC] Add 'override' keyword where missing in include/ and lib/.
Tue, Jul 14, 9:49 AM
logan-5 closed D83709: [NFC] Add 'override' keyword where missing in include/ and lib/..
Tue, Jul 14, 9:49 AM · Restricted Project
logan-5 committed rGfbb30c31fefc: [clang] Add 'override' to virtual function overrides generated by… (authored by logan-5).
[clang] Add 'override' to virtual function overrides generated by…
Tue, Jul 14, 9:39 AM
logan-5 closed D83616: [clang] Add 'override' to virtual function overrides generated by ClangAttrEmitter.
Tue, Jul 14, 9:39 AM · Restricted Project
logan-5 committed rG2c2a297bb6d1: [clang][NFC] Add 'override' keyword to virtual function overrides (authored by logan-5).
[clang][NFC] Add 'override' keyword to virtual function overrides
Tue, Jul 14, 9:03 AM

Mon, Jul 13

logan-5 added a comment to D83616: [clang] Add 'override' to virtual function overrides generated by ClangAttrEmitter.

Thanks! I would appreciate any help committing this.

Mon, Jul 13, 3:35 PM · Restricted Project

Jul 13 2020

logan-5 created D83709: [NFC] Add 'override' keyword where missing in include/ and lib/..
Jul 13 2020, 11:51 AM · Restricted Project

Jul 12 2020

logan-5 added a comment to D82728: [clang] Add -Wsuggest-override.

Looks good - thanks for the patch and all the details! Might be worth turning on by default in the LLVM build (after all the cleanup)

Thanks a lot! I don't (think I) have commit access yada yada, so I'd really appreciate any help getting this committed.

Jul 12 2020, 3:14 PM · Restricted Project

Jul 10 2020

logan-5 created D83616: [clang] Add 'override' to virtual function overrides generated by ClangAttrEmitter.
Jul 10 2020, 8:05 PM · Restricted Project
logan-5 added a reviewer for D83611: [clang][NFC] Add 'override' keyword to virtual function overrides: rsmith.
Jul 10 2020, 7:25 PM · Restricted Project
logan-5 created D83611: [clang][NFC] Add 'override' keyword to virtual function overrides.
Jul 10 2020, 7:24 PM · Restricted Project

Jul 9 2020

logan-5 added a comment to D82728: [clang] Add -Wsuggest-override.

Feels like a dumb question, but I'm not sure if and how those build failures are related to this patch? They seem to involve completely separate parts of the LLVM project (and .c files to boot). Was it that the build was failing for an unrelated reason at the moment the bots happened to build this patch? Or am I misunderstanding something more obvious?

Jul 9 2020, 10:04 AM · Restricted Project
logan-5 updated the summary of D82728: [clang] Add -Wsuggest-override.
Jul 9 2020, 10:00 AM · Restricted Project

Jul 7 2020

logan-5 added inline comments to D82728: [clang] Add -Wsuggest-override.
Jul 7 2020, 5:43 PM · Restricted Project
logan-5 added inline comments to D82728: [clang] Add -Wsuggest-override.
Jul 7 2020, 5:41 PM · Restricted Project
logan-5 updated the summary of D82728: [clang] Add -Wsuggest-override.
Jul 7 2020, 5:35 PM · Restricted Project
logan-5 updated the diff for D82728: [clang] Add -Wsuggest-override.

Fall back to -Wsuggest-override if -Winconsistent-missing-override is disabled.

Jul 7 2020, 3:47 PM · Restricted Project
logan-5 added a comment to D82728: [clang] Add -Wsuggest-override.

Oh, yep, there's a way - it's usually used for performance-costly warnings, to not spend the resources computing the warning if it's disabled anyway.

Wow, thanks--overlooked that in a big way. That'll definitely solve the problem. Fixed up patch on the way.

Jul 7 2020, 3:30 PM · Restricted Project
logan-5 updated the diff for D82728: [clang] Add -Wsuggest-override.

Addressed minor feedback.

Jul 7 2020, 1:55 PM · Restricted Project
logan-5 added a comment to D82728: [clang] Add -Wsuggest-override.

I think it might be nice to make the -Wno-inconsistent-missing-override -Wsuggest-override situation a bit better (by having it still do the same thing as -Wsuggest-override) but I don't feel /too/ strongly about it.

So, ironing this out would mean the code would need this structure:

if (Inconsistent && IsWarningEnabled(-Winconsistent-missing-override))
    Emit(-Winconsistent-missing-override);
else
    Emit(-Wsuggest-override);

The issue is that I wasn't able to find a way to ask if a warning is enabled and make a control flow decision based on that. If there is an API for doing this, it's hidden well. :) So I fell back to just doing if (Inconsistent) instead, which has this quirk if the inconsistent version of the warning is disabled.

Jul 7 2020, 12:27 PM · Restricted Project
logan-5 updated the diff for D82728: [clang] Add -Wsuggest-override.

clang-formatted the diff.

Jul 7 2020, 11:41 AM · Restricted Project
logan-5 added a comment to D82728: [clang] Add -Wsuggest-override.

Is the implementation you're proposing fairly consistent with GCC's? Run it over any big codebases to check it warns in the same places GCC does?

This patch has the same behavior as -Wsuggest-override in GCC >= 9. In GCC <9, it would suggest adding override to void foo() final, but in GCC >=9, final is enough to suppress the warning. This patch's -Wsuggest-override, as well as Clang's pre-existing -Winconsistent-missing-override, are also silenced by final. (https://godbolt.org/z/hbxLK6)

Jul 7 2020, 11:30 AM · Restricted Project
logan-5 added a comment to D82728: [clang] Add -Wsuggest-override.

Glad this is generating some discussion. For my $0.02, I would also (obviously) love to be able to enable this warning on all the codebases I work on, and this patch was born out of a discussion on the C++ Slack with another user who had found this warning very useful in GCC and was wondering why Clang didn't have it yet.

The issue is that such a warning then needs to be off by default, because we can't assume the user's intent. And Clang's historically been fairly averse to off-by-default warnings due to low user-penetration (not zero, like I said, I imagine LLVM itself would use such a warning, were it implemented) & so concerns about the cost/benefit tradeoff of the added complexity (source code and runtime) of the feature.

I agree -Wsuggest-override should be off by default, yet I suspect its user-penetration will be much higher than other off-by-default warnings, due to numerous instances of people on the Internet asking for this feature, as well as the precedent for it set by GCC. Moreover, since this implementation of this warning lies along the exact same code paths as the already existing -Winconsistent-missing-override, the added complexity from this patch is absolutely minimal.

Jul 7 2020, 12:49 AM · Restricted Project
logan-5 updated the diff for D82728: [clang] Add -Wsuggest-override.

Addressed some feedback.

Jul 7 2020, 12:21 AM · Restricted Project

Jun 28 2020

logan-5 updated the diff for D82728: [clang] Add -Wsuggest-override.

Don't warn for destructors by default. This makes -Wsuggest-[destructor-]override more consistent with the behavior of -Winconsistent-missing-[destructor-]override, as well as gcc.

Jun 28 2020, 8:20 PM · Restricted Project
logan-5 updated the diff for D82728: [clang] Add -Wsuggest-override.

Ran clang-format over the diff.

Jun 28 2020, 7:17 PM · Restricted Project
logan-5 created D82728: [clang] Add -Wsuggest-override.
Jun 28 2020, 1:27 PM · Restricted Project

May 8 2020

logan-5 added a comment to D69603: [libcxx] Add deduction guides for shared_ptr and weak_ptr.

Ack. It fails because there's a leak because a dummy deleter I used (D in deduction.pass.cpp) is a no-op. I suppose it should delete its argument as expected.

May 8 2020, 11:14 AM · Restricted Project

May 6 2020

logan-5 added a comment to D72640: [libcxx] Qualify make_pair in searcher implementations to prevent ADL.

Thanks. I'll need committing help, if this is good to go.

May 6 2020, 3:55 PM · Restricted Project
logan-5 added inline comments to D72640: [libcxx] Qualify make_pair in searcher implementations to prevent ADL.
May 6 2020, 10:46 AM · Restricted Project
logan-5 updated the diff for D72640: [libcxx] Qualify make_pair in searcher implementations to prevent ADL.

Formatting.

May 6 2020, 10:46 AM · Restricted Project
logan-5 added inline comments to D72640: [libcxx] Qualify make_pair in searcher implementations to prevent ADL.
May 6 2020, 1:34 AM · Restricted Project
logan-5 updated the diff for D72640: [libcxx] Qualify make_pair in searcher implementations to prevent ADL.

Pruned down tests to the essentials.

May 6 2020, 1:34 AM · Restricted Project

May 5 2020

logan-5 abandoned D74234: [clang] [NFC] Change boolean SuppressUserConversions parameters to an enum.
May 5 2020, 4:45 PM · Restricted Project
logan-5 added a comment to D69603: [libcxx] Add deduction guides for shared_ptr and weak_ptr.

Ping.

May 5 2020, 4:45 PM · Restricted Project
Herald added a reviewer for D72640: [libcxx] Qualify make_pair in searcher implementations to prevent ADL: Restricted Project.

Pinging this again.

May 5 2020, 4:45 PM · Restricted Project
logan-5 abandoned D74235: [clang] [NFC] Rename Sema::DiagnoseMultipleUserConversion to DiagnoseAmbiguousUserConversion.
May 5 2020, 4:45 PM · Restricted Project
logan-5 abandoned D74238: [clang] Improve diagnostic note for implicit conversion sequences that would work if more than one implicit user-defined conversion were allowed..

@rsmith the intention is to only speculatively search one level deep (e.g. search for chains like A->B, B->C, but no deeper), which should cover most of the cases that are surprising to users.

May 5 2020, 4:45 PM · Restricted Project

Mar 13 2020

logan-5 updated the diff for D72282: [clang-tidy] Add `bugprone-unintended-adl`.

Re-juggled and made bool OnlyDependsOnFundamentalArraySizes(QualType QualTy) recursive. Made :: assert more useful. Thanks @Quuxplusone for both good ideas.

Mar 13 2020, 12:23 PM · Restricted Project, Restricted Project

Mar 12 2020

logan-5 updated the diff for D72282: [clang-tidy] Add `bugprone-unintended-adl`.

The check now suggests fixes for concrete (i.e. non-template) uses of ADL. Re-enabled the check for macros, and eliminated some false positives with arrays of dependent size. The check now ignores hidden friends. Added a namespace whitelist. Added some tests and ensured that "don't-ignore-overloaded-operators" mode works properly in templates.

Mar 12 2020, 11:19 PM · Restricted Project, Restricted Project

Mar 11 2020

logan-5 added inline comments to D72282: [clang-tidy] Add `bugprone-unintended-adl`.
Mar 11 2020, 8:21 PM · Restricted Project, Restricted Project
logan-5 added a comment to D72282: [clang-tidy] Add `bugprone-unintended-adl`.

Thanks for the feedback.

  • This check should suggest fixes. It knows what function is actually being resolved, and it has enough information to create a minimally qualified name that resolves it.

True, though of course it will only be able to do so for the non-template version of the warning, since in the template version it only knows that the call _may_ be resolved through ADL.

  • This check should ignore hidden friends, since their entire purpose is to be called via ADL.
  • This check should allow whitelisting namespaces that opt-out of ADL into their namespace. This makes it much easier to roll out the clang-tidy incrementally.

I want to make absolutely sure I understand the last bullet. You'd like a whitelist of namespaces where, if a call is resolved by ADL to a function within any of those namespaces, the check doesn't fire?

Mar 11 2020, 7:38 PM · Restricted Project, Restricted Project
logan-5 added a comment to D72282: [clang-tidy] Add `bugprone-unintended-adl`.

Just giving this a friendly ping!

Mar 11 2020, 10:09 AM · Restricted Project, Restricted Project

Mar 4 2020

logan-5 added a comment to D74238: [clang] Improve diagnostic note for implicit conversion sequences that would work if more than one implicit user-defined conversion were allowed..

Pinging this and the patches it depends on. I figured it would need a rebase by now, but it still applies cleanly to trunk.

Mar 4 2020, 12:21 PM · Restricted Project

Feb 9 2020

logan-5 added a comment to D74290: [libcxx] Qualify make_move_iterator in vector::insert for input iterators.

I don't understand the actual algorithm being used in insert-with-input-iterators

Feb 9 2020, 6:00 PM · Restricted Project

Feb 8 2020

logan-5 created D74290: [libcxx] Qualify make_move_iterator in vector::insert for input iterators.
Feb 8 2020, 1:54 PM · Restricted Project
logan-5 added a comment to D74287: [libcxx] Fix unintended ADL inside ref(reference_wrapper<T>) and cref(reference_wrapper<T>).

PR44398 also reports an ADL ref in <memory> https://gcc.godbolt.org/z/EHw3Gy which this patch does not fix, unless I'm missing something.

Feb 8 2020, 1:09 PM · Restricted Project