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

Quuxplusone added a comment. In D76572#1936191 , @grandinj wrote: > Libreoffice has a similar clang-tidy-style cast-checker [for C-style casts] > here: > > https://cgit.freedesktop.org/libreoffice/core/tree/compilerplugins/clang/cstylecast.cxx > > https

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

Quuxplusone updated this revision to Diff 252031. Quuxplusone added a comment. @jhenderson: Updated! clang-format's suggested linebreaks look horrendous to me, but if this makes that buildbot happy, okay... I guess the moral of the story is "deeply nested expressions that also use lots of reinte

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

Quuxplusone added a comment. In D76572#1937558 , @rsmith wrote: > In D76572#1935791 , @lebedev.ri > wrote: > > > Passing-by remark: > > > > > I wrote a Clang warning [not pictured] to diagnose any use of `T(x)` >

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

Quuxplusone marked 2 inline comments as done. Quuxplusone added a comment. Btw, @jhenderson and @rsmith, if you wanted to land your own files' pieces of this patch while waiting on whoever-it-is to approve the pieces in other files, that'd be cool. As I mentioned initially, I don't have the acce

[PATCH] D74692: [clang-tidy] Make bugprone-use-after-move ignore std::move for const values

Quuxplusone added a comment. (I sent this to the mailing list, but I guess it doesn't show up here unless I do it through Phab. Quoting myself—) I see your point about how users who care should always be passing this check alongside "performance-move-const-arg"; but IMHO it still makes sense fo

[PATCH] D74692: [clang-tidy][bugprone-use-after-move] Warn on std::move for consts

Quuxplusone added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp:329 // CHECK-NOTES: [[@LINE-3]]:7: note: move occurred here + // CHECK-NOTES: [[@LINE-6]]:7: note: std::move of the const expression {{.*}} };

[PATCH] D74361: [Clang] Undef attribute for global variables

Quuxplusone added inline comments. Comment at: clang/test/CodeGenCXX/attr-no-zero-initializer.cpp:40 +// CHECK: @unt = global %struct.nontrivial undef +nontrivial unt [[clang::no_zero_initializer]]; Can you explain a bit about how this interacts with C++ construc

[PATCH] D74361: [Clang] Undef attribute for global variables

Quuxplusone added inline comments. Comment at: clang/test/CodeGenCXX/attr-loader-uninitialized.cpp:23 +// CHECK: @nominally_value_init = global i32 undef +int nominally_value_init [[clang::loader_uninitialized]] = 4; + This test case is identical to line 36 of

[PATCH] D74361: [Clang] Undef attribute for global variables

Quuxplusone added inline comments. Comment at: clang/test/CodeGenCXX/attr-loader-uninitialized.cpp:23 +// CHECK: @nominally_value_init = global i32 undef +int nominally_value_init [[clang::loader_uninitialized]] = 4; + rjmccall wrote: > JonChesterfield wrote: >

[PATCH] D74361: [Clang] Undef attribute for global variables

Quuxplusone added inline comments. Comment at: clang/test/CodeGenCXX/attr-loader-uninitialized.cpp:23 +// CHECK: @nominally_value_init = global i32 undef +int nominally_value_init [[clang::loader_uninitialized]] = 4; + rjmccall wrote: > JonChesterfield wrote: >

[PATCH] D68845: Don't emit unwanted constructor calls in co_return statements

Quuxplusone added inline comments. Comment at: clang/lib/Sema/SemaCoroutine.cpp:896 + if (S.CanPerformCopyInitialization(Entity, &AsRvalue)) +return true; +} else if (auto *FTD = dyn_cast(D)) { aaronpuchert wrote: > Overlad resolution can actuall

[PATCH] D58896: Suppress -Wchar-subscripts if the index is a literal char

Quuxplusone added a comment. In D58896#1737242 , @edward-jones wrote: > In D58896#1737200 , @xbolva00 wrote: > > > Well, i am not sure if one twitter report is good motivation to criple > > warning. > > > The moti

[PATCH] D69762: [Diagnostics] Try to improve warning message for -Wreturn-type

Quuxplusone added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:579 def warn_maybe_falloff_nonvoid_function : Warning< - "control may reach end of non-void function">, + "not all control paths in this function return a value; non-void function

[PATCH] D69762: [Diagnostics] Try to improve warning message for -Wreturn-type

Quuxplusone added a comment. In D69762#1737612 , @easyaspi314 wrote: > How about "this non-void {function|block} {may|does} not return a value" FWIW, I am happy with this clear and concise suggestion. :) Repository: rG LLVM Github Monorepo CHANGES S

[PATCH] D70390: [clang-tidy] new performance-no-automatic-move check.

Quuxplusone added a comment. Hi, I'm the main original author (although I would not claim "current maintainer") of the Clang warning. Clang doesn't warn about `const vector v; return v;` because in that case, adding `std::move` to the return would not help anything. I had not considered that we

[PATCH] D74692: [clang-tidy][bugprone-use-after-move] Warn on std::move for consts

Quuxplusone added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp:342 + // CHECK-NOTES: [[@LINE-5]]:20: note: variable 'a' implicitly captured const here }; } zinovy.nis wrote: > aaron.ballman wro

[PATCH] D74692: [clang-tidy][bugprone-use-after-move] Warn on std::move for consts

Quuxplusone added a comment. Anyway, I still don't see the point of this patch. It seems like you're just duplicating the work of `performance-move-const-arg`. People who want to be shown notes about moves-of-const-args should just enable that check instead. CHANGES SINCE LAST ACTION https:/

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

Quuxplusone added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.h:27 +class UnintendedADLCheck : public ClangTidyCheck { + const bool IgnoreOverloadedOperators; + const std::vector AllowedIdentifiers; EricWF wrote: > I th

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

Quuxplusone added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-unintended-adl.cpp:202 +void macro_test(T t) { +#define MACRO(x) find_if(x, x, x) + logan-5 wrote: > EricWF wrote: > > Arguably this is *exactly* the kind of code

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

Quuxplusone added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/UnintendedADLCheck.cpp:51 + // Remove the '::' at the end. + assert(NNS.size() >= 2); + NNS.erase(NNS.end() - 2, NNS.end()); As long as you're asserting anyway, I think you sh

[PATCH] D74692: [clang-tidy][bugprone-use-after-move] Warn on std::move for consts

Quuxplusone added a comment. I still think this entire patch is misguided; there's no reason to make the note for `const std::string s; std::move(s)` any longer than the note for `int i; std::move(i)` or `volatile std::string v; std::move(v)`. People should not be using moved-from objects, peri

[PATCH] D73441: [clang-tidy] Fix bugprone-use-after-move when move is in noexcept operator

Quuxplusone added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp:387 + unless(inDecltypeOrTemplateArg()), + unless(hasAncestor(cxxNoexceptExpr( .bind("call-move"); What about `si

[PATCH] D73548: [clang-tidy] Added option for disabling const qualifiers in readability-qualified-auto

Quuxplusone added inline comments. Comment at: clang-tools-extra/docs/clang-tidy/checks/readability-qualified-auto.rst:41 + auto *const Bar = cast(Baz2); + auto *volatile FooBar = cast(Baz3); + Is it worth adding an example of a double pointer? auto Bar

[PATCH] D73675: Avoid many std::tie/tuple instantiations in ASTImporter

Quuxplusone added inline comments. Herald added a subscriber: rnkovacs. Comment at: clang/lib/AST/ASTImporter.cpp:1152 + if (Error E = importSeq(ToElementType, ToSizeExpr)) +return std::move(E); As the author of [P1155 "More Implicit Move"](https://wg21.li

[PATCH] D73675: Avoid many std::tie/tuple instantiations in ASTImporter

Quuxplusone added inline comments. Comment at: clang/lib/AST/ASTImporter.cpp:1152 + if (Error E = importSeq(ToElementType, ToSizeExpr)) +return std::move(E); martong wrote: > Quuxplusone wrote: > > As the author of [P1155 "More Implicit Move"](https://wg21

[PATCH] D33844: [clang-tidy] terminating continue check

Quuxplusone added inline comments. Comment at: clang-tidy/misc/TerminatingContinueCheck.cpp:42 + + auto Diag = diag(ContStmt->getLocStart(), "terminating 'continue'"); + Diag << FixItHint::CreateReplacement(ContStmt->getSourceRange(), "break"); It was not clear

[PATCH] D43322: Diagnose cases of "return x" that should be "return std::move(x)" for efficiency

Quuxplusone added a comment. @rtrieu gentle ping! Action items I need help with, cut-and-pasted from above: - Ideally, test compiling a bunch of (e.g. Google) code with https://reviews.llvm.org/D43322, see if there are any rough edges - Decide if `-Wmove` should imply `-Wreturn-std-move` (I hav

[PATCH] D44883: [Sema] Extend -Wself-assign with -Wself-assign-overloaded to warn on overloaded self-assignment (classes)

Quuxplusone added inline comments. Comment at: lib/Sema/SemaExpr.cpp:12087 + case BO_AndAssign: + case BO_OrAssign: +DiagnoseSelfAssignment(S, LHS, RHS, OpLoc, /*IsBuiltin=*/false); I understand why `x &= x` and `x |= x` are mathematically special for the

[PATCH] D44883: [Sema] Extend -Wself-assign with -Wself-assign-overloaded to warn on overloaded self-assignment (classes)

Quuxplusone added a comment. Two more high-level comments from me, and then I'll try to butt out. Q1. I don't understand what `field-` is doing in the name of some diagnostics. Is there a situation where `x = x;` is "less/more of an error" just because `x` is a {data member, lambda capture, str

[PATCH] D43322: Diagnose cases of "return x" that should be "return std::move(x)" for efficiency

Quuxplusone updated this revision to Diff 139961. Quuxplusone edited the summary of this revision. Quuxplusone added a comment. Rebased on master. AFAIK this is ready to go and I just need someone to land it for me... @rtrieu ping? I believe the PR summary would be suitable as a commit message.

[PATCH] D44948: Add diagnostic -Waggregate-ctors, "aggregate type has user-declared constructors"

Quuxplusone created this revision. Herald added a subscriber: cfe-commits. This new warning diagnoses any aggregate class/struct/union type which has user-declared constructors, in order to test the hypothesis that real code does not (intentionally) contain such animals. If no real code

[PATCH] D44948: Add diagnostic -Waggregate-ctors, "aggregate type has user-declared constructors"

Quuxplusone planned changes to this revision. Quuxplusone added a comment. This is related to an upcoming C++ paper, definitely not ready for prime time (e.g. no tests), and quite possibly never will be ready (although if people thought it would be useful, I'd be happy to clean it up and add tes

[PATCH] D43322: Diagnose cases of "return x" that should be "return std::move(x)" for efficiency

Quuxplusone updated this revision to Diff 140103. Quuxplusone added a comment. Addressed review comments from @rsmith and rebased on master. I still need someone else to land this for me, though. Repository: rC Clang https://reviews.llvm.org/D43322 Files: include/clang/Basic/DiagnosticGrou

[PATCH] D45128: [libcxx][test] Silence -Wself-assign diagnostics

Quuxplusone added inline comments. Comment at: test/std/language.support/support.types/byteops/and.assign.pass.cpp:30 -static_assert(noexcept(b &= b), "" ); +static_assert(noexcept(b &= (std::byte &)b), "" ); lebedev.ri wrote: > EricWF wrote: > > Sho

[PATCH] D43322: Diagnose cases of "return x" that should be "return std::move(x)" for efficiency

Quuxplusone added a comment. @rsmith ping? Repository: rC Clang https://reviews.llvm.org/D43322 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45163: [Sema] -Wunused-value: diagnose unused std::move() call results.

Quuxplusone added a comment. > I think it wouldn't be unreasonable to ask standard library maintainers to > add `[[nodiscard]]` to `std::move` +1. Also `std::forward`, for sure. Basically any metaprogramming function that is statically known to return a reference to its argument. This knowledge

[PATCH] D45163: [Sema] -Wunused-value: diagnose unused std::move() call results.

Quuxplusone added a comment. `std::move` would definitely be special in this regard if there were a pressing benefit to be gained — i.e., if people were currently getting bitten by accidentally discarded calls of `std::move(x)`. But you haven't shown that people are getting bitten today; in fac

[PATCH] D44883: [Sema] Extend -Wself-assign and -Wself-assign-field to warn on overloaded self-assignment (classes)

Quuxplusone added a comment. > The warning is fundamentally about dataflow, but this doesn't apply to > unevaluated expressions. There are plenty of cases where a user might want to > ask if assignment is well formed on noexcept using only one variable. For > example: > > template void foo(

[PATCH] D45179: [libc++] Add _LIBCPP_FORCE_NODISCARD define to force-enable nodiscard in pre-C++17

Quuxplusone added inline comments. Comment at: test/libcxx/diagnostics/force_nodiscard.fail.cpp:15 +// MODULES_DEFINES: _LIBCPP_FORCE_NODISCARD +#define _LIBCPP_FORCE_NODISCARD +#include <__config> What is the purpose of `_LIBCPP_FORCE_NODISCARD`? On one of your

[PATCH] D45163: [Sema] -Wunused-value: diagnose unused std::move() call results.

Quuxplusone added a comment. In https://reviews.llvm.org/D45163#1055957, @lebedev.ri wrote: > In https://reviews.llvm.org/D45163#1055856, @rjmccall wrote: > > > Well, we should feel welcome to submit patches to the C++ library > > implementations, I think. I can understand why Marshall is waiti

[PATCH] D45179: [libc++] Add _LIBCPP_FORCE_NODISCARD define to force-enable nodiscard in pre-C++17

Quuxplusone added a comment. >> If we must have an opt-in/out mechanism (which I don't believe we do) > > Yes, we do. Opt-out is pre-existing, and removing it would be an > [unacceptable] regression. Ah, I see now that you weren't the originator of `_LIBCPP_DISABLE_NODISCARD_AFTER_CXX17`; that

[PATCH] D45179: [libc++] Add _LIBCPP_FORCE_NODISCARD define to force-enable nodiscard in pre-C++17

Quuxplusone added a comment. In https://reviews.llvm.org/D45179#1056706, @lebedev.ri wrote: > gcc does not silence the warning produced by > these attributes via `(void)` cast, unless it's `[[nodiscard]]` attribute :/ > [...] So until they fix that (they will fix that right? :)), > we can only

[PATCH] D43322: Diagnose cases of "return x" that should be "return std::move(x)" for efficiency

Quuxplusone updated this revision to Diff 141174. Quuxplusone added a comment. Finally learned how to "make check-clang" and actually run the test in the correct environment. Had to add `-fcxx-exceptions` to the command lines at the top of that file, because the code uses `throw`. @rsmith PTAL?

[PATCH] D45403: Make [[maybe_unused]] work with static data members

Quuxplusone added inline comments. Comment at: lib/Sema/SemaDeclAttr.cpp:2054 - } - // If this is spelled as the standard C++17 attribute, but not in C++17, warn @aaron.ballman writes: > I don't think we're currently diagnosing static data members of classes

[PATCH] D43322: Diagnose cases of "return x" that should be "return std::move(x)" for efficiency

Quuxplusone added a comment. Tuesday ping! I just need someone to commit this for me. Repository: rC Clang https://reviews.llvm.org/D43322 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe

[PATCH] D45179: [libc++] Add _LIBCPP_FORCE_NODISCARD define to force-enable nodiscard in pre-C++17

Quuxplusone added a comment. In https://reviews.llvm.org/D45179#1064589, @lebedev.ri wrote: > I'm waiting for @mclow.lists to have the final say re this differential. Ack. :) > So roughly: > > // NOTE: Do not use [[nodiscard]] in pre-C++17 mode > // to avoid -Wc++17-extensions warni

[PATCH] D40144: Implement `std::launder`

Quuxplusone added inline comments. Comment at: test/std/language.support/support.dynamic/ptr.launder/launder.fail.cpp:26 +int *p = nullptr; +std::launder(p); // expected-error {{ignoring return value of function declared with 'nodiscard' attribute}} +}

[PATCH] D27163: Introduce -f[no-]strict-return flag that controls code generation for C++'s undefined behaviour return optimisation

Quuxplusone added inline comments. Comment at: include/clang/Sema/AnalysisBasedWarnings.h:93 - void IssueWarnings(Policy P, FunctionScopeInfo *fscope, - const Decl *D, const BlockExpr *blkExpr); + void IssueWarnings(Policy P, FunctionScopeInfo *fscope, Dec

[PATCH] D27163: Introduce -f[no-]strict-return flag that controls code generation for C++'s undefined behaviour return optimisation

Quuxplusone added a comment. LGTM. I wonder if rsmith is happy with the exact semantics of "shouldUseUndefinedBehaviorReturnOptimization"... but that seems like a tiny nit that's fixable post-commit anyway. Comment at: include/clang/Driver/Options.td:1324 + HelpText<"Use C+

[PATCH] D27815: [clang-tidy] Add obvious module for obvious bugs

Quuxplusone added a comment. The provided example (typoing "i" for "j") sounds like the sort of thing that PVS-Studio catches; maybe see what wording they use for that kind of mistake? Without investigating, I would suggest "cut-and-paste-error" or "likely-typo". However, the attached patch *d

[PATCH] D27806: [clang-tidy] Add obvious-invalid-range

Quuxplusone added inline comments. Comment at: clang-tidy/obvious/InvalidRangeCheck.cpp:62 + auto CallsAlgorithm = hasDeclaration( + functionDecl(Names.size() > 0 ? hasAnyName(Names) : anything())); + Prazek wrote: > alexfh wrote: > > Does this check make

[PATCH] D27815: [clang-tidy] Add obvious module for obvious bugs

Quuxplusone added a comment. PVS-Studio implements tons of checks of this variety. See e.g. http://www.viva64.com/en/b/0299/#ID0E4KBM They don't have a catchy name for the category either, but perhaps "suspicious-" or "copypaste-" would do. I agree with Aaron that "thinko-" would be a little ina

[PATCH] D28148: [Sema] Suppress warnings for C's zero initializer

Quuxplusone added subscribers: rjmccall, Quuxplusone. Quuxplusone added a comment. Re the language option: In https://reviews.llvm.org/D27163, @rsmith wrote: > Perhaps some catch-all "I want defined behavior for things that C defines > even though I'm compiling in C++" flag would make sense here

[PATCH] D28467: [Sema] Add warning for unused lambda captures

Quuxplusone added inline comments. Comment at: test/SemaCXX/warn-unused-lambda-capture.cpp:17 + auto explicit_by_value_unused = [i] {}; // expected-warning{{lambda capture 'i' is not used}} + auto explicit_by_value_unused_sizeof = [i] { return sizeof(i); }; // expected-warnin

[PATCH] D28467: [Sema] Add warning for unused lambda captures

Quuxplusone added inline comments. Comment at: test/SemaCXX/warn-unused-lambda-capture.cpp:17 + auto explicit_by_value_unused = [i] {}; // expected-warning{{lambda capture 'i' is not used}} + auto explicit_by_value_unused_sizeof = [i] { return sizeof(i); }; // expected-warnin

[PATCH] D30837: [libcxx] Support for shared_ptr

Quuxplusone added inline comments. Comment at: include/memory:3933 +typedef __shared_ptr_pointer<_Yp*, default_delete<_Yp>, _AllocT > _CntrlBlk; +__cntrl_ = new _CntrlBlk(__p, default_delete<_Yp>(), _AllocT()); __hold.release(); EricWF wrote: > Thi

[PATCH] D34649: Remove addtional parameters in function std::next() and std::prev()

Quuxplusone added inline comments. Comment at: include/iterator:619 template inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14 Might fix the spelling error in "BidirectionalIter" while you're here. https://reviews.llvm.org/D34649 __

[PATCH] D35578: Add -fswitch-tables and -fno-switch-tables flags

Quuxplusone added a comment. If the goal is fine-grained control over the heuristics for compiling switch statements, perhaps one should enumerate all the possible ways to lower switch statements --- jump-tables, lookup-tables, if-trees, if-chains, (more?) --- and add a separate flag for each o

[PATCH] D35824: [Sema] Implicitly apply the may_alias attribute to std::byte

Quuxplusone added inline comments. Comment at: test/CodeGenCXX/std-byte.cpp:7 +enum byte : unsigned char {}; +} + Would it be worth adding an explicit test that `::byte`, `::my::byte`, `::my::std::byte` are or are not handled in the same way? https://reviews.l

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

Quuxplusone created this revision. `pointer_traits::to_pointer(r)` is not required to return a deallocatable pointer; indeed generally it *cannot* determine a deallocatable representation without help from the allocator. Therefore, we must not rely on representations derived from `to_pointer` w

[PATCH] D42248: Always allow "#pragma region".

Quuxplusone added a comment. FWIW, I would also like this patch, because it would mean that I could build with -Werror even when the project includes headers written by MSVC-using people. Given that we know what "#pragma region" does, it hardly deserves an "unknown pragma" diagnostic! So this p

[PATCH] D42693: [libcxx] Handle invalid escaped characters in POSIX regex

Quuxplusone added inline comments. Comment at: include/regex:3465 +case '{': +case '}': +break; FWIW, I don't understand what's going on in this switch. Is it intentional that `'('` and `'|'` now take different paths here?

[PATCH] D42730: Add clang-tidy check for use of types/classes/functions from header which are deprecated and removed in C++17

Quuxplusone added inline comments. Comment at: clang-tidy/modernize/AvoidFunctionalCheck.h:19 + +/// Check for several deprecated types and classes from header +/// aaron.ballman wrote: > alexfh wrote: > > alexfh wrote: > > > aaron.ballman wrote: > > > > massber

[PATCH] D41217: [Concepts] Concept Specialization Expressions

Quuxplusone added inline comments. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2404 +def err_concept_non_constant_constraint_expression : Error< + "concept specialization '%0' resulted in a non-constant expression '%1'.">; +def err_non_bool_atomic_constraint : Error<

[PATCH] D30837: [libcxx] Support for shared_ptr

Quuxplusone added a comment. I notice that the Standard implies that std::unique_ptr will work only with *object types* T, and yet the "object" wording is missing from shared_ptr. > A unique pointer is an object that owns another *object* and manages that > other *object* through a pointer.

[PATCH] D27163: Introduce -f[no-]strict-return flag that controls code generation for C++'s undefined behaviour return optimisation

Quuxplusone added inline comments. Comment at: lib/Sema/AnalysisBasedWarnings.cpp:530 + // Don't need to mark Objective-C methods or blocks since the undefined + // behaviour optimization isn't used for them. +} This seems like a trap waiting to spring on someo

[PATCH] D117535: [clang-tidy] Force LF newlines when writing files

Quuxplusone accepted this revision. Quuxplusone added a comment. This revision is now accepted and ready to land. LGTM FWIW. I might even wonder why this code is using `io.open` instead of just plain `open`. (I didn't know `io.open` was a thing; StackOverflow tells me that it was something in Py

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

Quuxplusone created this revision. Quuxplusone added reviewers: ldionne, rsmith, leonardchan, mizvekov. Quuxplusone added a project: clang. Quuxplusone requested review of this revision. Herald added a subscriber: cfe-commits. If this is a SFINAE context, then continuing to look up names (in parti

[PATCH] D114732: [clang] Mark `trivial_abi` types as "trivially relocatable".

Quuxplusone added a comment. In D114732#3253142 , @devin.jeanpierre wrote: > OK, while I'm struggling to set up a new Windows machine so I can make sure > this works on Windows... @Quuxplusone, after this is merged, do you want to > rebase D67524

[PATCH] D114732: [clang] Mark `trivial_abi` types as "trivially relocatable".

Quuxplusone added inline comments. Comment at: clang/test/SemaCXX/attr-trivial-abi.cpp:9-11 +// On Windows, trivial relocatability depends only on a trivial copy constructor existing. +// In this case, it is implicitly deleted. Similar concerns apply to later tests. +static_ass

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

Quuxplusone updated this revision to Diff 401652. Quuxplusone added a comment. Clang-formatted. @mizvekov or @rsmith: ping? I'd like to get this in before Feb 1, which is the release/14.x branch date AFAIK; because it affects workarounds in libc++ and thus makes a difference whether we will be

[PATCH] D92956: Fix range-loop-analysis checks for trivial copyability

Quuxplusone added a comment. What ever happened to this patch? @fanfuqiang, are you still around to rebase this? Do you need someone to commit it for you? (Not that I'm saying I think it's perfect as is — I haven't looked and don't know the code that well — but just wondering what kept this from

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

Quuxplusone added reviewers: ChuanqiXu, urnathan, hokein, dblaikie. Quuxplusone added a comment. Add some more reviewers who've touched SemaCXX recently. Ping! If no objections are forthcoming, I'd like to land this on Friday morning. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST AC

[PATCH] D115867: [C++20] [Coroutines] Warning for always_inline coroutine

Quuxplusone accepted this revision. Quuxplusone added a comment. This revision is now accepted and ready to land. LGTM now, modulo my suggested edits (and the necessary corresponding edits in the test case). I don't think I'm really qualified to accept, but as nobody else is looking, and my name

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

This revision was automatically updated to reflect the committed changes. Closed by commit rG9be5f4d5afd9: [clang] Don't typo-fix an expression in a SFINAE context. (authored by arthur.j.odwyer). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117603/n

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

Quuxplusone reopened this revision. Quuxplusone added a comment. This revision is now accepted and ready to land. I'm not sure if this caused https://lab.llvm.org/buildbot/#/builders/60/builds/6350 https://lab.llvm.org/buildbot/#/builders/119/builds/7433 but I'm acting as if it did. Anyone see ho

[PATCH] D118518: [clang][NFC] Change some ->getType()->isPlaceholderType() to just ->hasPlaceholderType()

Quuxplusone created this revision. Quuxplusone added reviewers: ChuanqiXu, urnathan, efriedma, kazu. Quuxplusone added a project: clang. Quuxplusone requested review of this revision. Herald added a subscriber: cfe-commits. While trying to figure out https://github.com/llvm/llvm-project/issues/529

[PATCH] D118518: [clang][NFC] Change some ->getType()->isPlaceholderType() to just ->hasPlaceholderType()

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG424400da2db8: [clang][NFC] Change some ->getType()->isPlaceholderType() to just… (authored by arthur.j.odwyer). Changed prior to commit: https://r

[PATCH] D118552: [clang] [concepts] Correctly(?) handle placeholder types in ExprRequirements.

Quuxplusone created this revision. Quuxplusone added reviewers: ChuanqiXu, saar.raz, rsmith. Quuxplusone added a project: clang. Quuxplusone requested review of this revision. Herald added a subscriber: cfe-commits. PR52905 was originally papered over in a different way, but I believe this is the

[PATCH] D118552: [clang] [concepts] Correctly(?) handle placeholder types in ExprRequirements.

Quuxplusone added a subscriber: rjmccall. Quuxplusone added inline comments. Comment at: clang/lib/AST/ASTContext.cpp:3375 +assert(T->isSpecificPlaceholderType(BuiltinType::UnknownAny) && "Unresolved placeholder type"); + } Btw, I strongly suspect that th

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

Quuxplusone updated this revision to Diff 404341. Quuxplusone added a comment. It seems like we need `tryToRecoverWithCall` to always identify a diagnostic explaining the problem, so just short-circuit-returning `ExprError()` with no diagnostic is not acceptable. Let's try this version instead.

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

Quuxplusone updated this revision to Diff 404343. Quuxplusone added a comment. Move the regression test to clang/test/SemaTemplate/, alongside the one for D118552 . Also, make it test the error message wording. CHANGES SINCE LAST ACTION https://reviews.llvm.

[PATCH] D118552: [clang] [concepts] Correctly(?) handle placeholder types in ExprRequirements.

Quuxplusone added inline comments. Comment at: clang/lib/AST/ASTContext.cpp:3375 +assert(T->isSpecificPlaceholderType(BuiltinType::UnknownAny) && "Unresolved placeholder type"); + } Quuxplusone wrote: > Btw, I strongly suspect that the presence of placeho

[PATCH] D118552: [clang] [concepts] Correctly(?) handle placeholder types in ExprRequirements.

Quuxplusone updated this revision to Diff 404380. Quuxplusone added a comment. Poke CI (clang-format failed). CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118552/new/ https://reviews.llvm.org/D118552 Files: clang/lib/AST/ASTContext.cpp clang/lib/Sema/SemaOverload.cpp clang/lib/S

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

Quuxplusone updated this revision to Diff 404381. Quuxplusone added a comment. Poke CI (clang-format failed). CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117603/new/ https://reviews.llvm.org/D117603 Files: clang/lib/Sema/Sema.cpp clang/lib/Sema/SemaExprMember.cpp clang/test/Sem

[PATCH] D118552: [clang] [concepts] Correctly(?) handle placeholder types in ExprRequirements.

Quuxplusone added a comment. Gentle ping! (@ChuanqiXu @urnathan perhaps?) I'm hoping to land D118552 followed by a re-land of D117603 , this week and //ideally// tomorrow. :) CHANGES SINCE LAST ACTION https://reviews.llvm.o

[PATCH] D118552: [clang] [concepts] Correctly(?) handle placeholder types in ExprRequirements.

Quuxplusone added inline comments. Comment at: clang/lib/AST/ASTContext.cpp:3373 ASTContext::getLValueReferenceType(QualType T, bool SpelledAsLValue) const { - assert(getCanonicalType(T) != OverloadTy && - "Unresolved overloaded function type"); + if (T->isPlaceholderT

[PATCH] D118552: [clang] [concepts] Correctly(?) handle placeholder types in ExprRequirements.

Quuxplusone updated this revision to Diff 404621. Quuxplusone added a comment. Update assert style. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118552/new/ https://reviews.llvm.org/D118552 Files: clang/lib/AST/ASTContext.cpp clang/lib/Sema/SemaOverload.cpp clang/lib/Sema/SemaTe

[PATCH] D118552: [clang] [concepts] Correctly(?) handle placeholder types in ExprRequirements.

Quuxplusone updated this revision to Diff 404624. Quuxplusone added a comment. Update another single-line `if` too. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118552/new/ https://reviews.llvm.org/D118552 Files: clang/lib/AST/ASTContext.cpp clang/lib/Sema/SemaOverload.cpp clang

[PATCH] D114903: [clang] Support array placement new in constant expressions

Quuxplusone added a comment. I have no special knowledge of this. Seems like "it can't be that easy," but I don't know. Comment at: clang/test/SemaCXX/cxx2a-constexpr-dynalloc.cpp:138 } static_assert(call_std_construct_at()); I would think you ought to hav

[PATCH] D118552: [clang] [concepts] Correctly(?) handle placeholder types in ExprRequirements.

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGf6ce45670789: [clang] Correctly(?) handle placeholder types in ExprRequirements. (authored by arthur.j.odwyer). Repository: rG LLVM Github Monorep

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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGc0185ffaec3c: [clang] Don't typo-fix an expression in a SFINAE context. (authored by arthur.j.odwyer). Repository: rG LLVM Github Monorepo CHANGE

[PATCH] D120086: Explicitly document that `__is_trivially_relocatable(T)` implies that `T` is implicit-lifetime.

Quuxplusone added a comment. FWIW, I think this is a good idea. I even think that the parentheses and "Note:" are too self-deprecating, and this deserves to be a bigger deal. Perhaps `__is_implicit_lifetime(T)` should be an intrinsic in its own right! Then you could say very concretely that `__

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

Quuxplusone added inline comments. Comment at: clang/test/CodeGenCXX/nrvo.cpp:1537 + } + return x; // FIXME: NRVO could happen if B == false, but doesn't +} Quuxplusone wrote: > Nit: `s/if/when/` Serendipitously, I just ran into almost this exact scenario in D1

[PATCH] D120454: clang/www: Add links to tracking issues for C++20 features

Quuxplusone added inline comments. Comment at: clang/www/cxx_status.html:106 +automatically fetch status information from the issue in the tracker. For Example: +No + HTML nit: t

[PATCH] D113393: [c++2b] Implement P0849R8 auto(x)

Quuxplusone added a comment. In D113393#3345275 , @aaron.ballman wrote: >> Also implemented decltype(auto)(x) (https://wg21.link/p0849r2) as a Clang >> extension. > > I'd like to better understand the use cases for this. WG21 considered this as > part

[PATCH] D120398: [format] follow up: Use unsigned char as the base of all enums in FormatStyle

Quuxplusone added a comment. > Any runtime penalty? (Usually compilers choose a faster underlying type for > enums, not smaller) > The decision is made in the header (so I think it will always be `int` until > this doesn't fit). To decide what is faster the compiler would need to know > all

[PATCH] D120629: [clang] Remove unused variable AllElementsInt. NFC.

Quuxplusone created this revision. Quuxplusone added reviewers: rnk, rsmith, sammccall. Quuxplusone added a project: clang. Quuxplusone requested review of this revision. Herald added a subscriber: cfe-commits. This has been unused ever since it was committed in b8a501ccf1. Repository: rG LLVM

[PATCH] D120629: [clang] Remove unused variable AllElementsInt. NFC.

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGd3db74eadbfc: [clang] Remove unused variable AllElementsInt. (authored by arthur.j.odwyer). Repository: rG LLVM Github Monorepo CHANGES SINCE LAS

[PATCH] D120398: [format] follow up: Use unsigned char as the base of all enums in FormatStyle

Quuxplusone added a comment. In D120398#3351998 , @MyDeveloperDay wrote: > Before this lands can we have a discussion about what clarity this gives us?, > because I think it makes the code more unreadable, but surely we are saving > just 7x(3 bytes) (t

<    1   2   3   4   5   6   7   >