[PATCH] D85545: [Diagnostics] Diagnose missing comma in string array initialization

Quuxplusone added a comment. LGTM; nothing further from me! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85545/new/ https://reviews.llvm.org/D85545 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailma

[PATCH] D85091: [Sema, CodeGen] Implement [[likely]] and [[unlikely]] in IfStmt

Quuxplusone added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:1697 +It's not allowed to annotate a statement with both ``likely`` and +``unlikely``. It's not recommended to annotate both branches of an ``if`` +statement with an attribute.

[PATCH] D85091: [Sema, CodeGen] Implement [[likely]] and [[unlikely]] in IfStmt

Quuxplusone added inline comments. Comment at: clang/test/SemaCXX/attr-likelihood.cpp:101 +} +#endif aaron.ballman wrote: > Mordante wrote: > > aaron.ballman wrote: > > > Mordante wrote: > > > > Quuxplusone wrote: > > > > > I'd like to see a case like `if (x) [[l

[PATCH] D79714: [Diagnostics] Restore -Wdeprecated warning when user-declared copy assignment operator is defined as deleted (PR45634)

Quuxplusone added inline comments. Comment at: clang/include/clang/Basic/DiagnosticGroups.td:158 +def DeprecatedCopy : DiagGroup<"deprecated-copy", [DeprecatedCopyUserProvided]>; +def DeprecatedCopyDtor : DiagGroup<"deprecated-copy-dtor", [DeprecatedCopyDtorUserProvided]>; def

[PATCH] D79714: [Diagnostics] Restore -Wdeprecated warning when user-declared copy assignment operator is defined as deleted (PR45634)

Quuxplusone added inline comments. Comment at: clang/test/SemaCXX/deprecated-copy.cpp:7 +#ifdef NO_USER_PROVIDED +// expected-no-diagnostics +#endif xbolva00 wrote: > xbolva00 wrote: > > Quuxplusone wrote: > > > I'm fairly confident this should just be two differ

[PATCH] D33776: [libcxx] LWG2221: No formatted output operator for nullptr

Quuxplusone added inline comments. Comment at: include/ostream:225 +basic_ostream& operator<<(nullptr_t) +{ return *this << (const void*)0; } + mclow.lists wrote: > lichray wrote: > > Oh, common, I persuaded the committee to allow you to print a `(null)`

[PATCH] D106394: [clang][pp] adds '#pragma include_instead'

Quuxplusone added a comment. > Hmm... I like prior art. That clangd supports it suggests that there's a > section of code I can look at for inspiration if we were to replace this > pragma with the IWYU comment-pragma (I wonder why they didn't just go with > `#pragma IWYU ...`?). Is it reasonabl

[PATCH] D106252: Make simple requirements starting with requires ill-formed in in requirement body

Quuxplusone requested changes to this revision. Quuxplusone added inline comments. This revision now requires changes to proceed. Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:810 +def err_requires_expr_in_simple_requirement : Error< + "requires expression in req

[PATCH] D117603: [clang] Don't typo-fix an expression in a SFINAE context

Quuxplusone added a comment. In D117603#3292657 , @var-const wrote: > @Quuxplusone Now that it's landed, do we still need `__workaround_52970` in > libc++? (see > https://github.com/llvm/llvm-project/blob/167b623a6af26802af47f455aaa3806faaa9da9e/libcxx/

[PATCH] D119094: [clang] Don't emit redundant warnings for 'return;'

Quuxplusone created this revision. Quuxplusone added reviewers: rsmith, sammccall, mizvekov, majnemer, riccibruno, clang. Quuxplusone added a project: clang. Quuxplusone requested review of this revision. Herald added a subscriber: cfe-commits. ...when the function declaration's return type is al

[PATCH] D113369: [clang-format] Extend SpaceBeforeParens for requires

Quuxplusone added inline comments. Comment at: clang/include/clang/Format/Format.h:3371 +/// If ``true``, put space between requires keyword in a requires clause and +/// opening parentheses, if there is one. +/// \code HazardyKnusperkeks wrote: > Qu

[PATCH] D119094: [clang] Don't emit redundant warnings for 'return;'

Quuxplusone planned changes to this revision. Quuxplusone added a comment. Unfortunately some existing tests fail: https://reviews.llvm.org/harbormaster/unit/view/2282838/ I haven't yet figured out why consteval functions are considered to have `FD->isInvalidDecl()`. There's also an Objective-C

[PATCH] D119094: [clang] Don't emit redundant warnings for 'return;'

Quuxplusone added a comment. In D119094#3301403 , @sammccall wrote: > In D119094#3301297 , @Quuxplusone > wrote: > >> Unfortunately some existing tests fail: >> https://reviews.llvm.org/harbormaster/unit/view/22

[PATCH] D119094: [clang] Don't emit redundant warnings for 'return;'

Quuxplusone updated this revision to Diff 406585. Quuxplusone added a comment. This revision is now accepted and ready to land. Fix the two failing test cases (in one case by fixing what seems to have been a typo in the test). CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119094/new/ h

[PATCH] D119184: [clang] [concepts] Check constrained-auto return types for void-returning functions

Quuxplusone created this revision. Quuxplusone added reviewers: sammccall, saar.raz, rsmith, mizvekov, majnemer, riccibruno. Quuxplusone added a project: clang. Quuxplusone requested review of this revision. Herald added a subscriber: cfe-commits. Fixes https://github.com/llvm/llvm-project/issues

[PATCH] D119094: [clang] Don't emit redundant warnings for 'return;'

Quuxplusone updated this revision to Diff 406838. Quuxplusone added a comment. Oops. I'd put `[clang] [test] Fix an apparent typo in SemaCXX/consteval-return-void.cpp` in a separate commit for hygiene purposes, and forgot to include that commit in this diff. Updated. Repository: rG LLVM Gith

[PATCH] D119184: [clang] [concepts] Check constrained-auto return types for void-returning functions

Quuxplusone updated this revision to Diff 406847. Quuxplusone added a comment. Update one additional test where the expected output has changed: auto& f() { } now produces "can't make a reference to void" instead of "can't deduce `auto&` from no return statements." I think this is a mild regr

[PATCH] D119351: Update all LLVM documentation mentioning runtimes in LLVM_ENABLE_PROJECTS

Quuxplusone added inline comments. Comment at: bolt/docs/OptimizingClang.md:228-236 $ CPATH=${TOPLEV}/stage1/install/bin/ -$ cmake -G Ninja ${TOPLEV}/llvm-project/llvm -DLLVM_TARGETS_TO_BUILD=X86 \ +$ cmake -G Ninja -S ${TOPLEV}/llvm-project/llvm -B ${TOPLEV}/stage2-prof-gen \ +

[PATCH] D119184: [clang] [concepts] Check constrained-auto return types for void-returning functions

Quuxplusone updated this revision to Diff 407284. Quuxplusone added a comment. Rebase; clang-format. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119184/new/ https://reviews.llvm.org/D119184 Files: clang/include/clang/Sema/Sema.h clang/lib/Se

[PATCH] D119351: Update all LLVM documentation mentioning runtimes in LLVM_ENABLE_PROJECTS

Quuxplusone added inline comments. Comment at: bolt/docs/OptimizingClang.md:228-236 $ CPATH=${TOPLEV}/stage1/install/bin/ -$ cmake -G Ninja ${TOPLEV}/llvm-project/llvm -DLLVM_TARGETS_TO_BUILD=X86 \ +$ cmake -G Ninja -S ${TOPLEV}/llvm-project/llvm -B ${TOPLEV}/stage2-prof-gen \ +

[PATCH] D119184: [clang] [concepts] Check constrained-auto return types for void-returning functions

Quuxplusone marked an inline comment as not done. Quuxplusone added inline comments. Comment at: clang/lib/Sema/SemaStmt.cpp:3816 +Context.getTrivialTypeSourceInfo(Context.VoidTy, ReturnLoc), ReturnLoc); +Expr *Dummy = R.get(); +DeduceAutoResult DAR = DeduceAutoT

[PATCH] D119138: [clang-format] Further improve support for requires expressions

Quuxplusone added inline comments. Comment at: clang/lib/Format/UnwrappedLineParser.cpp:2798-2802 +// This one is really tricky, since most tokens doesn't have a type yet. +// The & or && can be binary operators, then we have a requires +// expression. But they also c

[PATCH] D119670: [clang] Warn on unqualified calls to std::move and std::forward

Quuxplusone added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:4812 +def warn_unqualified_call_to_std_function : Warning< + "unqualified call to %0">, InGroup>; + This should probably be named `-Wunqualified-std-move` resp. `-Wu

[PATCH] D119670: [clang] Warn on unqualified calls to std::move and std::forward

Quuxplusone added a comment. Tangential data point: I hacked up this check to find //all// unqualified `std` functions and ran it on libc++'s tests and also on some of my own codebase (including re2, asio, protobuf). I'm fixing the issues it turned up in libc++'s tests. In the other codebases,

[PATCH] D113369: [clang-format] Extend SpaceBeforeParens for requires

Quuxplusone added inline comments. Comment at: clang/docs/ClangFormatStyleOptions.rst:4007 + * ``bool AfterRequiresKeywordInRequiresClause`` If ``true``, put space between requires keyword in a requires clause and +opening parentheses, if there is one. cu

[PATCH] D119670: [clang] Warn on unqualified calls to std::move and std::forward

Quuxplusone added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:6437 + NamedDecl *D = dyn_cast_or_null(Call->getCalleeDecl()); + if (!D || !D->isInStdNamespace()) +return; cor3ntin wrote: > erichkeane wrote: > > Quuxplusone wrote: > > > erichkean

[PATCH] D119094: [clang] Don't emit redundant warnings for 'return;'

This revision was automatically updated to reflect the committed changes. Closed by commit rG3c8d2aa87c17: [clang] Don't emit redundant warnings for 'return;' (authored by arthur.j.odwyer). Changed prior to commit: https://reviews.llvm.org/D119094?vs=406838&id=408434#toc Repository: rG LLVM

[PATCH] D119772: [clang] [NFC] More exhaustive tests for deducing void return types

Quuxplusone created this revision. Quuxplusone added reviewers: sammccall, rsmith, dblaikie, mizvekov. Quuxplusone added a project: clang. Quuxplusone requested review of this revision. Herald added a subscriber: cfe-commits. This is a preliminary ahead of D119184

[PATCH] D119184: [clang] [concepts] Check constrained-auto return types for void-returning functions

Quuxplusone updated this revision to Diff 408594. Quuxplusone marked an inline comment as done and an inline comment as not done. Quuxplusone added a comment. Address review comments; add exhaustive tests as a parent revision. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119184/new/ ht

[PATCH] D119778: [clang] Add a note "deducing return type for 'foo'"

Quuxplusone created this revision. Quuxplusone added reviewers: sammccall, rsmith, dblaikie, majnemer, mizvekov, ChuanqiXu. Quuxplusone added a project: clang. Quuxplusone requested review of this revision. Herald added a subscriber: cfe-commits. Emit the note when we fail to deduce a return type

[PATCH] D119792: [Clang] [P2025] Analyze only potential scopes for NRVO

Quuxplusone added a reviewer: mizvekov. Quuxplusone added a comment. The code is above my pay grade, but FWIW, I super support the intent of this patch! Let's get p2025 support into Clang! :) > The collection of common cases contains 20 examples: §4.1. Examples. Here is > the current status of

[PATCH] D119893: [clang-format] Fixed handling of requires clauses followed by attributes

Quuxplusone added inline comments. Comment at: clang/lib/Format/UnwrappedLineParser.cpp:3018-3019 do { +bool LambdaThisTimeAllowed = LambdaNextTimeAllowed; +LambdaNextTimeAllowed = false; + Nit: For this pattern, consider `bool LambdaThisTimeAllowed =

[PATCH] D119184: [clang] [concepts] Check constrained-auto return types for void-returning functions

Quuxplusone updated this revision to Diff 409090. Quuxplusone added a comment. Rebase; fix the clang-format nit; reinsert a missing `FD->setInvalidDecl()` that seemed to be causing crashes in Modules code under libcxx/test/ but otherwise doesn't seem to be tested in clang/test/ :( Repository:

[PATCH] D119778: [clang] Add a note "deducing return type for 'foo'"

Quuxplusone updated this revision to Diff 409095. Quuxplusone added a comment. Rebase; adjust more expected output in tests. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119778/new/ https://reviews.llvm.org/D119778 Files: clang/include/clang/Ba

[PATCH] D119778: [clang] Add a note "deducing return type for 'foo'"

Quuxplusone added inline comments. Comment at: clang/lib/Sema/SemaStmt.cpp:3805 +if (DAR != DAR_Succeeded) { + if (OrigResultType.getBeginLoc().isValid()) +Diag(OrigResultType.getBeginLoc(), diag::note_deducing_return_type_for) > I am curious abo

[PATCH] D119778: [clang] Add a note "deducing return type for 'foo'"

Quuxplusone added a comment. I'll update with the `CodeSynthesisContext` approach in a little bit. But meanwhile there's a problem with both the current patch //and// (I think even more) with the new approach. @rsmith any help?: When I try my latest patch with the libc++ tests, with `bin/llvm-li

[PATCH] D119927: [Clang] [P2025] More exhaustive tests for NRVO

Quuxplusone added a comment. Excellent! Comment at: clang/test/CodeGenCXX/nrvo.cpp:622 + if (b) +return x; + else Similar to what you did in `test5`, I think it'd be helpful to comment //on the return line itself// whether NRVO is (done|possible|impossib

[PATCH] D119778: [clang] Add a note "deducing return type for 'foo'"

Quuxplusone updated this revision to Diff 409359. Quuxplusone added a comment. Apply @rsmith's suggested approach. > I don't see any obvious way that this patch could be responsible for that > failure, unless it's something like a pre-existing use of uninitialized > memory or a use-after-free a

[PATCH] D119927: [Clang] [P2025] More exhaustive tests for NRVO

Quuxplusone accepted this revision. Quuxplusone added a comment. This revision is now accepted and ready to land. LGTM FWIW. You might want to wait for someone more authoritative to take a look; but it's also only adding test coverage, so it seems pretty uncontroversial, I would think. ==

[PATCH] D119184: [clang] [concepts] Check constrained-auto return types for void-returning functions

Quuxplusone added inline comments. Comment at: clang/lib/Sema/SemaStmt.cpp:3766 +Expr *&RetExpr, const AutoType *AT, +bool HasReturnStmt) { + // If this is the conversion function for a lambd

[PATCH] D119184: [clang] [concepts] Check constrained-auto return types for void-returning functions

Quuxplusone added inline comments. Comment at: clang/test/SemaCXX/deduced-return-type-cxx14.cpp:116 -auto &void_ret_2() {} // expected-error {{cannot deduce return type 'auto &' for function with no return statements}} +auto &void_ret_2() {} // expected-error {{cannot form a r

[PATCH] D119772: [clang] [NFC] More exhaustive tests for deducing void return types

This revision was automatically updated to reflect the committed changes. Closed by commit rG7adb85884b35: [clang] [NFC] More exhaustive tests for deducing void return types (authored by arthur.j.odwyer). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/

[PATCH] D119184: [clang] [concepts] Check constrained-auto return types for void-returning functions

Quuxplusone updated this revision to Diff 409740. Quuxplusone added a comment. Rebase and update — this is becoming more and more of a trivial patch, which I guess is good! Add a test case for https://github.com/llvm/llvm-project/issues/53911 (which I finally thought to test, and was surprised t

[PATCH] D66397: [Diagnostics] Improve -Wxor-used-as-pow

Quuxplusone added a comment. In D66397#1639840 , @xbolva00 wrote: > Swap trick is cool, we can suggest it always(vs. ‘xor’) Well, you should avoid suggesting that the user rewrite `2 ^ 10` as `10 ^ 2`. Anyway, I think some of the observers here might be

[PATCH] D66397: [Diagnostics] Improve -Wxor-used-as-pow

Quuxplusone added a comment. @thakis wrote: > What was the motivation for firing on more than bare literals? Well, fundamentally, there is no difference in code quality between any of these snippets: #define BIG 300 double bigpower1() { return 10 ^ BIG; } static constexpr int BIG = 30

[PATCH] D66711: [clang] Warning for non-final classes with final destructors

Quuxplusone added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2204 "abstract class is marked '%select{final|sealed}0'">, InGroup; +def warn_final_destructor_nonfinal_class : Warning< + "class with destructor marked '%select{final|sealed}0' c

[PATCH] D35941: Fix -Wshadow false positives with function-local classes.

Quuxplusone accepted this revision. Quuxplusone added a comment. > But if I'm overseeing reasons to issue a warning in this case, we could add > an analogue of `-Wshadow-uncaptured-local` for this case. WDYT? I still think "any warning" is a marginally better UX than "no warning" on the particu

[PATCH] D35863: Use the allocator's pointers for deallocation in `std::list`

Quuxplusone updated this revision to Diff 109678. Quuxplusone added a comment. I've updated https://reviews.llvm.org/D35863 to be actually correct AFAICT from my local testing; but I'm not sure what's the most appropriate way to get "fancy allocator" tests into libcxx's test suite. The way I did

[PATCH] D50119: P1144 "Trivially relocatable" (0/3): Compiler support for `__is_trivially_relocatable(T)`

Quuxplusone added a comment. @rjmccall ping? any further thoughts on `[[maybe_trivially_relocatable]]`, in light of the followup libc++ patches? (Those patches do not use `[[maybe_trivially_relocatable]]`, although that may just be because I wrote them.) How could one go about getting this pat

[PATCH] D50119: P1144 "Trivially relocatable" (0/3): Compiler support for `__is_trivially_relocatable(T)`

Quuxplusone added a comment. In D50119#1808616 , @rjmccall wrote: > If the committee is actively looking into this problem and considering > multiple alternatives, I don't see a particular need to accept your proposal > into Clang in advance of the commi

[PATCH] D72282: [clang-tidy] Add `bugprone-unintended-adl`

Quuxplusone added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.cpp:43 + Whitelist( + utils::options::parseStringList(Options.get("Whitelist", "swap"))) {} + JonasToth wrote: > JonasToth wrote: > > logan-5 wro

[PATCH] D72282: [clang-tidy] Add `bugprone-unintended-adl`

Quuxplusone added a comment. Repeating for emphasis: This is awesome. :) Having the opt-in //option// to not-ignore overloaded operators is good; please keep that option (in case anyone tries to talk you into removing it). For more than you wanted to know about ADL, why it's bad for library wri

[PATCH] D72282: [clang-tidy] Add `bugprone-unintended-adl`

Quuxplusone added subscribers: zoecarver, CaseyCarter, ldionne, EricWF, mclow.lists. Quuxplusone added a comment. Re which libc++ folks could give feedback on this ADL-diagnosing patch: I don't know precisely, but the candidates are few! @mclow.lists @ericwf @ldionne @zoecarver @CaseyCarter.

[PATCH] D88220: [C++20] Implement more implicit moves for return statements(Part of P1825R0)

Quuxplusone added a comment. I'd like to see some indication in the code comments, in the commit message, and/or in cxx_status.html, explaining //what// parts of P1825 are implemented and what parts remain unimplemented after this patch. (My ideal would be to ho

[PATCH] D88295: [Sema] Fix volatile check when test if a return object can be implicitly move

Quuxplusone added a comment. (This patch was split out from D88220 at my request.) @nullptr.cpp, please add the regression test from https://godbolt.org/z/5EfK99 . After a test is added, this patch LGTM (but will need approval also from someone else). Your s

[PATCH] D86559: [Sema, CodeGen] Allow [[likely]] and [[unlikely]] on labels

Quuxplusone added a comment. This feels like the wrong approach to me... but I admit that I don't know what the "right" approach might be. (I doubt any right approach exists.) if (ch == ' ') [[likely]] { goto whitespace; // A } else if (ch == '\n' || ch == '\t') [[unlikely]] { g

[PATCH] D85091: [Sema, CodeGen] Implement [[likely]] and [[unlikely]] in IfStmt

Quuxplusone added inline comments. Comment at: clang/include/clang/AST/Stmt.h:1180 + + /// \returns the likelihood of the branches of an if statement. + static Likelihood getLikelihood(const Stmt *Then, const Stmt *Else); Mordante wrote: > aaron.ballman wrote:

[PATCH] D92886: [Sema] Warn about unused functions even when they're inline

Quuxplusone added subscribers: mpark, Quuxplusone. Quuxplusone added a comment. I agree with your reasoning, but in practice I still see a lot of people using `static inline` for functions (especially function templates) in .h files. I'm not sure exactly why — maybe to reduce the burden on the l

[PATCH] D92936: [Sema] Fix deleted function problem in implicitly movable test

Quuxplusone added inline comments. Comment at: clang/lib/Sema/SemaStmt.cpp:3110 +/// +/// \returns Whether need to do the second overload resolution. If the first +/// overload resolution fails, or if the first overload resolution success but s/need/we need/ ==

[PATCH] D92956: Fix https://bugs.llvm.org/show_bug.cgi?id=48011

Quuxplusone added inline comments. Comment at: clang/test/SemaCXX/warn-range-loop-analysis.cpp:35 + // operator. + ID1& operator=(ID1 const& other) { return *this; } +}; I recommend adding `ID1(ID1 const&) = default;` here. My understanding is that C++20 has d

[PATCH] D92024: [clang] Implement P0692R1 from C++20 (access checking on specializations and instantiations)

Quuxplusone added a comment. > And I came to that we can get rid of this hack and simplify the patch pretty > much Wow, this certainly looks massively better than it was before! :) Glad I could help. ;) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.o

[PATCH] D92361: [trivial-abi] Support types without a copy or move constructor.

Quuxplusone added inline comments. Comment at: clang/lib/Sema/SemaDeclCXX.cpp:6502 + // except that it has a non-trivial member *with* the trivial_abi attribute. + for (auto Base : D->bases()) { +if (auto CxxRecord = Base.getType()->getAsCXXRecordDecl()) zo

[PATCH] D76572: Replace `T(x)` with `reinterpret_cast(x)` everywhere it means reinterpret_cast. No functional change

Quuxplusone updated this revision to Diff 313433. Quuxplusone edited the summary of this revision. Quuxplusone added a comment. Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Rebase in preparation for finally pushing this thing if the buildbots seem happy. Repository:

[PATCH] D76572: Replace `T(x)` with `reinterpret_cast(x)` everywhere it means reinterpret_cast. No functional change

Quuxplusone updated this revision to Diff 313446. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76572/new/ https://reviews.llvm.org/D76572 Files: clang/lib/CodeGen/CGCall.h llvm/include/llvm/IR/SymbolTableListTraits.h llvm/include/llvm/Object/

[PATCH] D76572: Replace `T(x)` with `reinterpret_cast(x)` everywhere it means reinterpret_cast. No functional change

This revision was automatically updated to reflect the committed changes. Closed by commit rG22cf54a7fba6: Replace `T(x)` with `reinterpret_cast(x)` everywhere it means… (authored by arthur.j.odwyer). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/

[PATCH] D92024: [clang] Implement P0692R1 from C++20 (access checking on specializations and instantiations)

Quuxplusone added inline comments. Comment at: clang/lib/Parse/ParseTemplate.cpp:265 + // We don't use `SuppressAccessChecks` here because + // it supresses ALL the dignostics kinds, but we need to keep some of them. + // For instance marked as unavailable: Sp

[PATCH] D92936: [Sema] Fix deleted function problem in implicitly movable test

Quuxplusone accepted this revision. Quuxplusone added a comment. This revision is now accepted and ready to land. I don't fully understand the new control flow, but at least the new //behavior// (after applying this patch) looks like an improvement to me. I recommend rebasing on top-of-tree, main

[PATCH] D88295: [Sema] Fix volatile check when test if a return object can be implicitly move

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG703038b35a86: [Sema] Fix volatile check when testing if a return object can be implicitly… (authored by nullptr.cpp, committed by arthur.j.odwyer).

[PATCH] D91311: Add new 'preferred_name' attribute.

Quuxplusone added inline comments. Comment at: libcxx/include/regex:2520 +_LIBCPP_PREFERRED_NAME(wregex) +basic_regex { Why does this attribute go on the class template? Shouldn't it be an attribute on the typedef, so that you don't have to repeat yours

[PATCH] D91488: Consider reference, pointer, and pointer-to-membber TemplateArguments to be different if they have different types.

Quuxplusone added inline comments. Comment at: clang/lib/AST/ItaniumMangle.cpp:4861 +// We need correct types when the template-name is unresolved or when it +// might be overloaded. +if (!ResolvedTemplate) And from the PR summary: > namely mangling

[PATCH] D91311: Add new 'preferred_name' attribute.

Quuxplusone added inline comments. Comment at: libcxx/include/regex:2520 +_LIBCPP_PREFERRED_NAME(wregex) +basic_regex { aaron.ballman wrote: > rsmith wrote: > > Quuxplusone wrote: > > > Why does this attribute go on the class template? Shouldn't it be an

[PATCH] D91610: [clangd] Add OverridenBy Relation to index.

Quuxplusone added inline comments. Comment at: clang-tools-extra/clangd/index/Relation.cpp:21 + case RelationKind ::OverridenBy: +return OS << "OverridenBy"; + } Also here. And there's a bogus extra space in `RelationKind ::BaseOf`. Comme

[PATCH] D49317: Move __construct_forward (etc.) out of std::allocator_traits.

Quuxplusone abandoned this revision. Quuxplusone added a comment. Abandoning, as this has been done in d9a4f936d05 . Repository: rCXX libc++ CHANGES SINCE LAST ACTION https://reviews.llvm.org/D49317/new/ https://reviews.

[PATCH] D78938: Make LLVM build in C++20 mode

Quuxplusone added inline comments. Comment at: llvm/include/llvm/IR/BasicBlock.h:324-325 +template ::value>> phi_iterator_impl(const phi_iterator_impl &Arg) dblaikie wrote: > BRevzin wrote: > > dblaikie wrote: > > > What tripped over/required this SFINAE

[PATCH] D88220: [C++20] P1825R0: More implicit moves

Quuxplusone added inline comments. Comment at: clang/include/clang/Sema/Sema.h:4626 CES_AllowExceptionVariables = 4, -CES_FormerDefault = (CES_AllowParameters), -CES_Default = (CES_AllowParameters | CES_AllowDifferentTypes), -CES_AsIfByStdMove = (CES_AllowParamet

[PATCH] D92039: [-Wcalled-once-parameter] Introduce 'called_once' attribute

Quuxplusone added a comment. Probably irrelevant comment from the C++ world: If I needed this concept in C++, I'd probably piggyback on the existing semantic analysis of `std::move`: I'd design a `class OnceCallable` with an `operator() &&` that both called and nulled-out its object parameter,

[PATCH] D92039: [-Wcalled-once-parameter] Introduce 'called_once' attribute

Quuxplusone added inline comments. Comment at: clang/lib/Analysis/CalledOnceCheck.cpp:1223 +// We consider '(void)parameter' as a manual no-op escape. +// It should be used to explicitly tell the analysis that this paramater +// is intentionally not called on this pat

[PATCH] D91895: [Clang] improve -Wimplicit-fallthrough GCC compat

Quuxplusone added a comment. In D91895#2417170 , @kees wrote: > I think this should warn too. While this won't turn into a "missing break" > error, there's no way to know (from looking at code) what the _intent_ is > here. Hear, hear. +1 on everything @

[PATCH] D92267: [clang-tidy][NFC] Use moves instead of copies when constructing OptionsProviders.

Quuxplusone added inline comments. Comment at: clang-tools-extra/clang-tidy/ClangTidyOptions.h:176 public: - DefaultOptionsProvider(const ClangTidyGlobalOptions &GlobalOptions, - const ClangTidyOptions &Options) - : GlobalOptions(GlobalOptions), Def

[PATCH] D92267: [clang-tidy][NFC] Use moves instead of copies when constructing OptionsProviders.

Quuxplusone added inline comments. Comment at: clang-tools-extra/clang-tidy/ClangTidyOptions.h:225 + ClangTidyOptions &&OverrideOptions, + ConfigFileHandlers &&ConfigHandlers); njames93 wrote: > Quuxplusone wrot

[PATCH] D88220: [C++20] P1825R0: More implicit moves

Quuxplusone added a subscriber: david_stone. Quuxplusone added inline comments. Comment at: clang/test/CXX/class/class.init/class.copy.elision/p3.cpp:22 + return c; +} +#else @rsmith @david_stone (or anyone), what is the status in C++20 of the following test ca

[PATCH] D88220: [C++20] P1825R0: More implicit moves

Quuxplusone added a subscriber: davidstone. Quuxplusone added inline comments. Comment at: clang/test/CXX/class/class.init/class.copy.elision/p3.cpp:22 + return c; +} +#else Quuxplusone wrote: > @rsmith @david_stone (or anyone), what is the status in C++20 of th

[PATCH] D92024: [clang] Implement P0692R1 from C++20 (access checking on specializations and instantiations)

Quuxplusone added a comment. The subject line says "access checking on specializations and instantiations," but I don't see any tests for explicit instantiations here — just specializations. In particular, I'm very interested to know if P0692 is intended to have any effect on the legality of ht

[PATCH] D92361: [trivial-abi] Support types without a copy or move constructor.

Quuxplusone added a comment. > You're right; thinking about it in the context of four value operations is > helpful. FWIW, I think dragging "value operations" into the mix is exactly wrong (and referring to "destructive move" is extra wrong, in the specific context of C++). For a C++ object, w

[PATCH] D92361: [trivial-abi] Support types without a copy or move constructor.

Quuxplusone added a comment. In D92361#2433190 , @rjmccall wrote: > There is no such thing as an object "teleporting" in C++. Objects are always > observed in memory with a specific address. When an argument is passed in > registers, its address can be

[PATCH] D92671: Add diagnostic for for-range-declaration being specificed with thread_local

Quuxplusone added a comment. Spoilsport. ;) LGTM, fwiw. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92671/new/ https://reviews.llvm.org/D92671 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/li

[PATCH] D92671: Add diagnostic for for-range-declaration being specificed with thread_local

Quuxplusone added inline comments. Comment at: clang/lib/Sema/SemaDecl.cpp:12752 Error = 4; break; } Incidentally, why is this case here to begin with? Loop variables can totally be `register`. (But I guess it's moot at this point; clearly no Clang

[PATCH] D88220: [C++20] P1825R0: More implicit moves

Quuxplusone added a comment. I've found another case that Clang has never handled correctly (and which your patch does not fix)— https://godbolt.org/z/xd8qGW struct Widget {}; struct Frodo { Frodo(Widget&); Frodo(Widget&&) = delete; }; Frodo twelve() { Widget w;

[PATCH] D88220: [C++20] P1825R0: More implicit moves

Quuxplusone added inline comments. Comment at: clang/include/clang/Sema/Sema.h:4626 CES_AllowExceptionVariables = 4, -CES_FormerDefault = (CES_AllowParameters), -CES_Default = (CES_AllowParameters | CES_AllowDifferentTypes), -CES_AsIfByStdMove = (CES_AllowParamet

[PATCH] D92024: [clang] Implement P0692R1 from C++20 (access checking on specializations and instantiations)

Quuxplusone added inline comments. Comment at: clang/lib/Parse/ParseTemplate.cpp:172 + + // TODO. This can produce wrong detection in case of a later class + // declaration. Example: I don't know the purpose of this code, but this //seems// like a super import

[PATCH] D91245: [clang-format] Recognize c++ coroutine keywords as unary operator to avoid misleading pointer alignment

Quuxplusone added a comment. Peanut gallery says: It seems to me that your root problem here is that `co_yield` is not being recognized by the parser as a keyword, and so you're seeing e.g. `co_yield* p;` the same way you'd see `CoYield* p;`. But in that case, you should also be seeing `co_yield

[PATCH] D88295: [Sema] Fix volatile check when test if a return object can be implicitly move

Quuxplusone added inline comments. Comment at: clang/test/SemaCXX/implicitly-movable.cpp:14 + +private: + A(const A &); aaronpuchert wrote: > Is this testing what we want it to test? The private functions are just not > part of the overload set, right? > > I t

[PATCH] D88295: [Sema] Fix volatile check when test if a return object can be implicitly move

Quuxplusone added a comment. I believe the organization of that directory implies that the path should be `class/class.init/class.copy.elision/`, not `special/class.copy/`. Otherwise LGTM, and I like this new test better than the old one! I still can't help re actually landing this patch, as I do

[PATCH] D88295: [Sema] Fix volatile check when test if a return object can be implicitly move

Quuxplusone added a comment. > So, how about we add another `CES` flag As the original author of `CES_AsIfByStdMove`, I am opposed to any attempt to complicate this patch. My medium-term goal, now that P1155 has been adopted, is to //eliminate// the complexity

[PATCH] D47111: : Implement monotonic_buffer_resource.

Quuxplusone abandoned this revision. Quuxplusone marked 3 inline comments as done. Quuxplusone added a comment. Sure; not that D89057 is making any //more// progress... :) Repository: rCXX libc++ CHANGES SINCE LAST ACTION https://reviews.llvm.org/D47111/ne

[PATCH] D47358: : Implement {un,}synchronized_pool_resource.

Quuxplusone abandoned this revision. Quuxplusone added a comment. Superseded by D89057 . Repository: rCXX libc++ CHANGES SINCE LAST ACTION https://reviews.llvm.org/D47358/new/ https://reviews.llvm.org/D47358 ___

[PATCH] D47360: Implement as a copy of .

Quuxplusone abandoned this revision. Quuxplusone added a comment. Superseded by D89057 . Repository: rCXX libc++ CHANGES SINCE LAST ACTION https://reviews.llvm.org/D47360/new/ https://reviews.llvm.org/D47360 ___

[PATCH] D88220: [C++20] P1825R0: More implicit moves

Quuxplusone added inline comments. Comment at: clang/lib/Sema/SemaStmt.cpp:3226-3227 +CopyElisionSemanticsKind CESK = CES_Strict; +if (getLangOpts().CPlusPlus20) { + CESK = CES_ImplicitlyMovableCXX20; +} else if (getLangOpts().CPlusPlus11) { rsmi

[PATCH] D88220: [C++20] P1825R0: More implicit moves

Quuxplusone added inline comments. Comment at: clang/lib/Sema/SemaStmt.cpp:3226-3227 +CopyElisionSemanticsKind CESK = CES_Strict; +if (getLangOpts().CPlusPlus20) { + CESK = CES_ImplicitlyMovableCXX20; +} else if (getLangOpts().CPlusPlus11) { rsmi

[PATCH] D102760: [llvm] Let SmallVector construct from any Iterable

Quuxplusone added inline comments. Comment at: llvm/include/llvm/ADT/IterableTraits.h:17-31 +namespace detail { + +template auto is_forward_iterator(...) -> std::false_type; + +template +auto is_forward_iterator(int) -> decltype( +std::next(std::declval()),

<    1   2   3   4   5   6   7   >