[PATCH] D72860: [modules] Do not cache invalid state for modules that we attempted to load.

rsmith added inline comments. Comment at: clang/lib/Serialization/ModuleManager.cpp:183 // Get a buffer of the file and close the file descriptor when done. - Buf = FileMgr.getBufferForFile(NewModule->File, /*isVolatile=*/false); + Buf = FileMgr.getBufferForFile(

[PATCH] D73852: [clang] detect switch fallthrough marked by a comment (PR43465)

rsmith added a comment. In D73852#1901836 , @aaron.ballman wrote: > FYI: it was reverted by Luboš in c61401b89742f230b7e6a2cc0548fbf7442e906d > Thank you, Luboš, and sorry for the pr

[PATCH] D73852: [clang] detect switch fallthrough marked by a comment (PR43465)

rsmith added a comment. In D73852#1901699 , @llunak wrote: > In D73852#1901186 , @rsmith wrote: > > > We shouldn't enable the warning under -Wextra in language modes where > > there's no standard way to suppress it

[PATCH] D73852: [clang] detect switch fallthrough marked by a comment (PR43465)

rsmith added a comment. Per http://llvm.org/PR43465#c37, this patch has not had proper review and we have not established consensus that we want this. Please revert for now. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73852/new/ https://reviews.

[PATCH] D73852: [clang] detect switch fallthrough marked by a comment (PR43465)

rsmith added a comment. I'm also pretty concerned about using comments to drive warning behavior. We discussed this when first adding our gallery fallthrough warning and its suppression mechanism and made an explicit decision that we did not want comments to affect our behaviour. I don't think

[PATCH] D75383: clang: Switch C compilations to C17 by default.

rsmith accepted this revision. rsmith added a comment. Looks good. Please consider updating the Clang 10 release notes to indicate that we intend to change the default in Clang 11. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75383/new/ https://r

[PATCH] D73020: [Sema] Perform call checking when building CXXNewExpr

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. In D73020#1891368 , @lebedev.ri wrote: > In D73020#1857704 , @rsmith wrote: > > > Looks good if you change the

[PATCH] D74684: [Sema][C++] Adopt DR2171 relaxed triviality requirements

rsmith added a comment. This change should be guarded by `-fclang-abi-compat=` so that users can opt to using the old ABI. Comment at: clang/lib/Sema/SemaDeclCXX.cpp:9200-9202 - // C++11 [class.copy]p12, p25: [DR1593] - // A [special member] is trivial if [...] its paramet

[PATCH] D71734: [Modules] Handle tag types and complain about bad merges in C/Objective-C mode

rsmith added inline comments. Comment at: clang/lib/AST/DeclCXX.cpp:487 // Only calculate hash on first call of getODRHash per record. - ODRHash Hash; + class ODRHash Hash; Hash.AddCXXRecordDecl(getDefinition()); I think this change is no longer necessary

[PATCH] D74643: [Parse] Consider attributes in `TryParsePtrOperatorSeq`.

rsmith added inline comments. Comment at: clang/lib/Parse/ParseTentative.cpp:773 + while (Tok.isOneOf(tok::l_square, tok::kw___attribute, tok::kw___declspec, + tok::kw_alignas)) { +if (Tok.is(tok::l_square)) { aaron.ballman wrote: > Do we

[PATCH] D74643: [Parse] Consider attributes in `TryParsePtrOperatorSeq`.

rsmith accepted this revision. rsmith added inline comments. Comment at: clang/include/clang/Parse/Parser.h:2444 + /// Try to skip a possibly empty sequence of 'attribute-specifier' without of + /// full validation of the syntactic structure of attributes. St

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

rsmith added a comment. In D71920#1870195 , @sammccall wrote: > If you'd like to have `concept::Requirement` use a similar bitfield, I'd just > like to confirm my understanding of the current code before refactoring it: > > - there's just one `Dependent`

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

rsmith added a comment. In D71920#1884740 , @sammccall wrote: > @rsmith Friendly ping. Do you want to review this/other patches in this > sequence, or should we be a bit bolder and pull you in when we're uncertain > in principle what to do? I'm fine wi

[PATCH] D73245: Depend stddef.h to provide max_align_t for C++11 and provide better fallback in

rsmith added a comment. On the assumption that we will never get a `::max_align_t` in C++98 mode anyway (which will be the case if the `` is conforming), this looks like the best we can do to me. Comment at: libcxx/include/new:229-237 +#if !defined(_LIBCPP_CXX03_LANG) +using

[PATCH] D74051: Move update_cc_test_checks.py tests to clang

rsmith added a comment. In D74051#1872856 , @MaskRay wrote: > @rsmith Does `clang/test/utils/` (a new directory) look appropriate to you? That seems fine to me. Following that pattern, I think we should also move: - `test/clang-rename` -> `test/tools/cl

[PATCH] D73637: Fix handling of OO_Spaceship in DecodeOperatorCall

rsmith added a comment. An equivalent patch was landed as llvmorg-11-init-2132-gb96c6b65b93 to fix PR44786. Thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73637/new/ https://reviews.llvm.org/D73637 ___ cfe-commits mailing list cfe-c

[PATCH] D71241: [OpenMP][WIP] Use overload centric declare variants

rsmith added a comment. In D71241#1788003 , @jdoerfert wrote: > In D71241#1787652 , @hfinkel wrote: > > > In D71241#1787571 , @ABataev wrote: > > > > > In D71241#1787265

[PATCH] D69272: Enable '#pragma STDC FENV_ACCESS' in frontend

rsmith requested changes to this revision. rsmith added a comment. This revision now requires changes to proceed. I don't see any changes to the constant evaluator here. You need to properly handle constant evaluation within `FENV_ACCESS ON` contexts, somehow, or you'll miscompile floating-point

[PATCH] D74103: Implement P1766R1: diagnose giving non-C-compatible classes a typedef name for linkage purposes.

This revision was automatically updated to reflect the committed changes. Closed by commit rG7ae1b4a0ce9c: Implement P1766R1: diagnose giving non-C-compatible classes a typedef name for… (authored by rsmith). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.

[PATCH] D74103: Implement P1766R1: diagnose giving non-C-compatible classes a typedef name for linkage purposes.

rsmith added a comment. Ugh, you mentioned template argument lists and made me realize we can still reach problem cases that way. And with array bounds. :( I've added back in a diagnostic for the case where we compute the linkage too early for a "C-like" struct. Hopefully people won't hit it to

[PATCH] D74103: Implement P1766R1: diagnose giving non-C-compatible classes a typedef name for linkage purposes.

rsmith updated this revision to Diff 243057. rsmith marked an inline comment as done. rsmith added a comment. Add back a diagnostic for remaining cases that the new langauge rule doesn't handle. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74103/n

[PATCH] D74103: Implement P1766R1: diagnose giving non-C-compatible classes a typedef name for linkage purposes.

rsmith updated this revision to Diff 243048. rsmith marked 3 inline comments as done. rsmith added a comment. Extend assert comment to suggest likely remediations. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74103/new/ https://reviews.llvm.org/D7

[PATCH] D74103: Implement P1766R1: diagnose giving non-C-compatible classes a typedef name for linkage purposes.

rsmith added inline comments. Comment at: clang/lib/Sema/SemaDecl.cpp:4394 +isa(D)) + continue; + rjmccall wrote: > This is essentially part of the next paragraph. > > I believe `friend` declarations also do not declare "members", under the same >

[PATCH] D74103: Implement P1766R1: diagnose giving non-C-compatible classes a typedef name for linkage purposes.

rsmith updated this revision to Diff 243022. rsmith marked 4 inline comments as done. rsmith added a comment. Updates based on review comments from @rjmccall. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74103/new/ https://reviews.llvm.org/D74103

[PATCH] D74103: Implement P1766R1: diagnose giving non-C-compatible classes a typedef name for linkage purposes.

rsmith updated this revision to Diff 243023. rsmith added a comment. Add test coverage for the 'friend' case. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74103/new/ https://reviews.llvm.org/D74103 Files: clang/docs/ReleaseNotes.rst clang/inc

[PATCH] D73967: Implement _ExtInt as an extended int type specifier.

rsmith added a comment. In D73967#1861757 , @erichkeane wrote: > Extended-vector types don't really make sense for non-powers-of-two (plus > have some odd gotchas when it comes to vectors of i1 for example), so I've > added a test that shows that this is

[PATCH] D74103: Implement P1766R1: diagnose giving non-C-compatible classes a typedef name for linkage purposes.

rsmith created this revision. rsmith added a reviewer: rjmccall. Herald added a project: clang. Herald added a subscriber: cfe-commits. Due to a recent (but retroactive) C++ rule change, only sufficiently C-compatible classes are permitted to be given a typedef name for linkage purposes. Add an en

[PATCH] D73967: Implement _ExtInt as an extended int type specifier.

rsmith added a comment. Have you considered how this would interact with our other language extensions? Can we form a vector of `_ExtInt(N)`? A `_Complex _ExtInt(N)`? I expect for some of that to just fall out of the implementation, but we should have documentation and test coverage either way.

[PATCH] D68896: PR43080: Do not build context-sensitive expressions during name classification.

rsmith added a comment. In D68896#1861040 , @rsmith wrote: > In D68896#1778193 , @kianm wrote: > > > Hi, I am still seeing problems with this assertion. Could we please get a > > fix? I've posted the reduced test c

[PATCH] D68896: PR43080: Do not build context-sensitive expressions during name classification.

rsmith added a comment. In D68896#1778193 , @kianm wrote: > Hi, I am still seeing problems with this assertion. Could we please get a > fix? I've posted the reduced test case and reproducible command on this > Phabricator patch. Are you still seeing pr

[PATCH] D71734: [Modules] Handle tag types and complain about bad merges in C/Objective-C mode

rsmith added inline comments. Comment at: clang/include/clang/AST/Decl.h:4009-4010 + + /// Store the ODR hash for this decl. + unsigned ODRHash; }; We shouldn't store this here; this will make all `CXXRecordDecl`s larger to store a field they will never look

[PATCH] D74009: [clang] Improve diagnostic note for implicit conversions that are disallowed because they involve more than one user-defined conversion.

rsmith added a comment. This is an interesting idea. Have you looked into whether you can defer the check for conversion via multiple user-defined conversions until we come to call `CompleteNonViableCandidate` after finding no viable functions? That'd give me a lot more confidence that this won

[PATCH] D73020: [Sema] Perform call checking when building CXXNewExpr

rsmith added a comment. Looks good if you change the error to a warning. Comment at: clang/lib/Sema/SemaChecking.cpp:3887-3891 if (!I.isPowerOf2()) { Diag(Arg->getExprLoc(), diag::err_alignment_not_power_of_two) << Arg->getSourceRange();

[PATCH] D72231: [Sema] Adds the pointer-to-int-cast diagnostic

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Looks fine. Would it make sense to put the MS extension warning into the `-Wpointer-to-int-cast` group so that we can control this warning consistently across platforms? You could get that ef

[PATCH] D73020: [Sema] Perform call checking when building CXXNewExpr

rsmith added a comment. It would be really useful to provide these extra parameters to `FindAllocationFunctions` too. For example, we could support direct dispatch to a size-class-specific allocation function: void *operator new(std::size_t n) __attribute__((enable_if(n == 16, "optimized all

[PATCH] D67678: PR17164: Change clang's default behavior from -flax-vector-conversions=all to -flax-vector-conversions=integer.

rsmith added a comment. In D67678#1836953 , @dexonsmith wrote: > In D67678#1836922 , @rsmith wrote: > > > If there's no timeline to update the macOS SDK, then perhaps we could add a > > hack to Clang to allow these

[PATCH] D63960: [C++20] Add consteval-specific semantic for functions

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Some nits and very minor things, but I think this is generally good to go with those fixed. Sorry for the long review delay, and thanks for working on it! Comment at: clang/

[PATCH] D68923: Don't warn about missing declarations for partial template specializations

rsmith accepted this revision. rsmith added a comment. Sorry I missed this before now. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68923/new/ https://reviews.llvm.org/D68923 ___ cfe-commits mailing l

[PATCH] D73701: [clang] fix linkage of nested lambda

rsmith added a comment. This is effectively reverting https://reviews.llvm.org/D1783. If we don't need that any more for whatever reason, can we also get rid of the check for whether the outermost lambda has a mangling number a few lines above? (Please also check if we can remove `getOutermostE

[PATCH] D73245: Don't define std::max_align_t if not used in C++03 mode

rsmith added a comment. libc++ intentionally provides all the C++11 library functionality that it can, even when used from C++03 mode. So on the face of it, providing this name in C++03 mode seems appropriate. However... the implementation currently used by libc++ **doesn't work** in that mode

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

rsmith added a comment. I don't like the name `getDependencies`, because the function is not getting a list of dependencies, it's getting flags that indicate whether certain properties of the construct are dependent. Maybe `getDependence` or `getDependenceFlags` would be a better name? Likewise

[PATCH] D73380: [clang] Annotating C++'s `operator new` with more attributes

rsmith added inline comments. Comment at: clang/lib/Sema/SemaDecl.cpp:14361 +/// attributes are applied to declarations. +void Sema::AddKnownFunctionAttributesForReplaceableGlobalAllocationFunction( +FunctionDecl *FD) { lebedev.ri wrote: > rsmith wrote: > > T

[PATCH] D73380: [clang] Annotating C++'s `operator new` with more attributes

rsmith added inline comments. Comment at: clang/lib/Sema/SemaDecl.cpp:14361 +/// attributes are applied to declarations. +void Sema::AddKnownFunctionAttributesForReplaceableGlobalAllocationFunction( +FunctionDecl *FD) { This should all be done by CodeGen, not

[PATCH] D73388: NFC: Implement AST node skipping in ParentMapContext

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. This seems reasonable to me. If you want to unify something about this and the `Ignore*` functions in `Expr`, maybe we could add a "classify" mechanism, so you could ask "what kind of node is

[PATCH] D73029: Extend ExprTraversal class with acending traversal

rsmith added a comment. We're working on moving the parent map out of `ASTContext` and into something specific to tooling; please don't add more dependencies onto it in the AST library. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73029/new/ htt

[PATCH] D73028: Extract ExprTraversal class from Expr

rsmith added a comment. In D73028#1839494 , @steveire wrote: > In D73028#1839383 , @rsmith wrote: > > > > > > The follow-up is here: https://reviews.llvm.org/D73029 . > > I have the need to change the ascending trav

[PATCH] D73028: Extract ExprTraversal class from Expr

rsmith added a comment. > An addition to the API will be concerned with ascending through the AST in > different traversal modes. Ascending through the AST is not possibly by design. (For example, we share AST nodes between template instantiations, and statement nodes don't contain parent poin

[PATCH] D72242: Fix crash on value dependent bitfields in if conditions in templates

rsmith requested changes to this revision. rsmith added a comment. This revision now requires changes to proceed. This is not an appropriate fix; the code is locally correct. The right way to handle this is exactly what Clang currently does: if the expression is not type-dependent, then contextu

[PATCH] D67678: PR17164: Change clang's default behavior from -flax-vector-conversions=all to -flax-vector-conversions=integer.

rsmith added a comment. In D67678#1836668 , @dexonsmith wrote: > In D67678#1836628 , @steven_wu wrote: > > > In D67678#1834957 , @rsmith wrote: > > > > > In D67678#1834542

[PATCH] D73150: [Concepts] Remove -fconcepts-ts, enable concepts support under -std=c++2a

rsmith added inline comments. Comment at: clang/include/clang/Basic/LangOptions.def:124 ENUM_LANGOPT(LaxVectorConversions, LaxVectorConversionKind, 2, - LaxVectorConversionKind::All, "lax vector conversions") + LaxVectorConversionKind::Integer, "lax vecto

[PATCH] D73153: [Concepts] Update ReleaseNotes with Concepts support

rsmith accepted this revision. rsmith added inline comments. This revision is now accepted and ready to land. Comment at: clang/docs/ReleaseNotes.rst:127-128 +- The -fno-concept-satisfaction-caching can be used to disable caching for + satisfactions of Concepts. Using this flag

[PATCH] D67678: PR17164: Change clang's default behavior from -flax-vector-conversions=all to -flax-vector-conversions=integer.

rsmith added a comment. In D67678#1834957 , @rsmith wrote: > I'll split out the new flag at least and re-land for Clang 10, while we > figure out how to set the default. Looks like there's nothing to re-land here: only the change of default was reverte

[PATCH] D67678: PR17164: Change clang's default behavior from -flax-vector-conversions=all to -flax-vector-conversions=integer.

rsmith added a comment. In D67678#1834542 , @steven_wu wrote: > @rsmith This also breaks macOS SDK. Can we revert this as we discuss a less > aggressive option? (This was already reverted a couple of days ago.) Do you have a timeline for how long it wo

[PATCH] D72897: List implicit operator== after implicit destructors in a vtable.

rsmith added a comment. In D72897#1832234 , @vvereschaka wrote: > Hi Richard, > > http://lab.llvm.org:8011/builders/llvm-clang-win-x-armv7l builder has been > broken by this patch during few days (failed "Clang::virtual-compare.cpp" > test). > Sorry, bu

[PATCH] D72552: [Concepts] Constraint Satisfaction Caching

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Mechanically, this looks fine. There's an ongoing discussion in the committee as to whether this kind of caching is permissible. But if this is necessary for acceptable performance, let's tak

[PATCH] D72897: List implicit operator== after implicit destructors in a vtable.

This revision was automatically updated to reflect the committed changes. Closed by commit rGadd2b7e44ada: List implicit operator== after implicit destructors in a vtable. (authored by rsmith). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72897/new/

[PATCH] D65042: [Concept] Placeholder constraints and abbreviated templates

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. I've left some comments suggesting how to rebase this on rGa42fd84cff265b7e9faa3fe42885ee171393e4db ; otherwise, some mino

[PATCH] D72897: List implicit operator== after implicit destructors in a vtable.

rsmith updated this revision to Diff 238881. rsmith added a comment. - Explicitly put implicit virtual functions in the right order, rather Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72897/new/ https://reviews.llvm.org/D72897 Files: clang/lib

[PATCH] D72897: List implicit operator== after implicit destructors in a vtable.

rsmith added a comment. In D72897#1825791 , @rjmccall wrote: > I know that Sema used to have a lot of issues with implicitly declaring > special members, but I also know that that was bound up with how we tracked > special state for the class, and that c

[PATCH] D50360: [Concepts] Requires Expressions

rsmith accepted this revision. rsmith added inline comments. This revision is now accepted and ready to land. Comment at: clang/include/clang/Parse/Parser.h:804-807 + bool TryAnnotateTypeOrScopeToken(bool ClassTemplateDeductionContext = true); + bool TryAnnotateTypeOrScopeToken

[PATCH] D72897: List implicit operator== after implicit destructors in a vtable.

rsmith created this revision. rsmith added a reviewer: rjmccall. Herald added a project: clang. Herald added a subscriber: cfe-commits. We previously listed first declared members, then implicit operator=, then implicit operator==, then implicit destructors. Per discussion on https://github.com/it

[PATCH] D65042: [Concept] Placeholder constraints and abbreviated templates

rsmith added a comment. I think this might work out more cleanly if we represented a constrained auto type as a `ConstrainedType` node wrapping an `AutoType` node rather than putting both things into the same object. (This will become more pressing if/when C++ starts allowing, for example, cons

[PATCH] D50360: [Concepts] Requires Expressions

rsmith added inline comments. Comment at: clang/include/clang/AST/ExprConcepts.h:144-148 + struct SubstitutionDiagnostic { +StringRef SubstitutedEntity; +SourceLocation DiagLoc; +StringRef DiagMessage; + }; Please add a FIXME to store the diagnostic

[PATCH] D50360: [Concepts] Requires Expressions

rsmith added inline comments. Comment at: clang/lib/AST/StmtPrinter.cpp:2296 + S.flush(); + if (Buf.find("typename ") != 0) +OS << "typename "; I can see why this is the most straightforward way to implement this, but ... yuck. Please add a FIX

[PATCH] D67678: PR17164: Change clang's default behavior from -flax-vector-conversions=all to -flax-vector-conversions=integer.

This revision was automatically updated to reflect the committed changes. Closed by commit rGb72a8c65e4e3: PR17164: Change clang's default behavior from -flax-vector-conversions=all to… (authored by Richard Smith , committed by rsmith). Changed prior to commit: http

[PATCH] D67678: PR17164: Change clang's default behavior from -flax-vector-conversions=all to -flax-vector-conversions=integer.

rsmith added a comment. Herald added a subscriber: wuzish. Ping, I don't think I have any actionable feedback here and I'd like to get this landed before Clang 10 branches. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67678/new/ https://reviews.l

[PATCH] D67678: PR17164: Change clang's default behavior from -flax-vector-conversions=all to -flax-vector-conversions=integer.

rsmith updated this revision to Diff 238099. rsmith added a comment. Herald added subscribers: kbarton, nemanjai. Rebase and slightly expand release notes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67678/new/ https://reviews.llvm.org/D67678 Fi

[PATCH] D44352: [Concepts] Type Constraints

rsmith accepted this revision. rsmith added a comment. Some cleanups (mainly parameters that are no longer necessary), then this is good to go. Thanks! Comment at: clang/include/clang/Basic/LangOptions.def:146 LANGOPT(DllExportInlines , 1, 1, "dllexported classes dllexport i

[PATCH] D50360: [Concepts] Requires Expressions

rsmith added a comment. (Partial comments; I'll try to complete the review today or tomorrow.) Comment at: clang/include/clang/AST/ExprCXX.h:4933 + +/// \brief A static requirement that can be used in a requires-expression to You're adding a total of ~400 lin

[PATCH] D72565: adding __has_feature support for left c++ type_traits

rsmith requested changes to this revision. rsmith added inline comments. This revision now requires changes to proceed. Comment at: clang/include/clang/Basic/Features.def:191-192 // Type traits // N.B. Additional type traits should not be added to the following list. // Instea

[PATCH] D72411: [CodeGen] partially revert 2b4fa5348ee157b6b1a1af44d0137ca8c7a71573 to fix Objective-C static variable initialization

rsmith added inline comments. Comment at: clang/lib/CodeGen/CGDecl.cpp:377 - if (D.needsDestruction(getContext()) && HaveInsertPoint()) { + if (hasNontrivialDestruction(D.getType()) && HaveInsertPoint()) { // We have a constant initializer, but a nontrivial destructor. W

[PATCH] D72411: [CodeGen] partially revert 2b4fa5348ee157b6b1a1af44d0137ca8c7a71573 to fix Objective-C static variable initialization

rsmith added inline comments. Comment at: clang/lib/CodeGen/CGDecl.cpp:377 - if (D.needsDestruction(getContext()) && HaveInsertPoint()) { + if (hasNontrivialDestruction(D.getType()) && HaveInsertPoint()) { // We have a constant initializer, but a nontrivial destructor. W

[PATCH] D44352: [Concepts] Type Constraints

rsmith added inline comments. Comment at: clang/include/clang/AST/DeclTemplate.h:1297-1300 + if (TC->hasExplicitTemplateArgs()) +for (const auto &ArgLoc : TC->getTemplateArgsAsWritten()->arguments()) + if (ArgLoc.getArgument().containsUnexpandedParameterPack

[PATCH] D44352: [Concepts] Type Constraints

rsmith added inline comments. Comment at: include/clang/AST/ASTNodeTraverser.h:518 +if (const auto *TC = D->getTypeConstraint()) + if (TC->wereArgumentsSpecified()) +for (const auto &ArgLoc : TC->getTemplateArgsAsWritten()->arguments()) For consi

[PATCH] D43357: [Concepts] Function trailing requires clauses

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Thanks, I'm happy to iterate on this further after you commit (though feel free to ask for another round of review if you prefer). The changes to recovery from missing parens in a trailing req

[PATCH] D70172: [CUDA][HIP] Fix assertion due to dtor check on windows

rsmith added a comment. Herald added a subscriber: herhut. This doesn't look quite right to me. I don't think we should treat the `delete this;` for a destructor as being emitted-for-device in any translation unit in which the vtable is marked used. (For example, if in your testcase `MSEmitDele

[PATCH] D43357: [Concepts] Function trailing requires clauses

rsmith added inline comments. Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:3440-3447 + // C++2a [temp.deduct]p5 + // [...] When all template arguments have been deduced [...] all uses of + // template parameters [...] are replaced with the corresponding deduced +

[PATCH] D43357: [Concepts] Function trailing requires clauses

rsmith marked an inline comment as done. rsmith added inline comments. Comment at: clang/include/clang/AST/ASTNodeTraverser.h:391-392 +if (const Expr *TRC = D->getTrailingRequiresClause()) + Visit(TRC); + It would be better to do this before we visit t

[PATCH] D71572: [ItaniumCXXABI] Make tls wrappers comdat on Windows

rsmith added inline comments. Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:2551 + // actually handle multiple TUs defining the same wrapper. + if (CGM.getTriple().isOSWindows() && CGM.supportsCOMDAT() && + Wrapper->isWeakForLinker()) mstorsjo wrote: > m

[PATCH] D71714: [Sema] Fix -Warray-bounds false negative when casting an out-of-bounds array item

rsmith added a comment. In D71714#1795313 , @ilya wrote: > While I agree with the general notion about the flaws with the current > implementation, I feel that the more major refactoring proposed in this > review is out of the scope of this minor fix.

[PATCH] D71714: [Sema] Fix -Warray-bounds false negative when casting an out-of-bounds array item

rsmith added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:13384 case Stmt::MemberExprClass: { expr = cast(expr)->getBase(); break; ilya wrote: > rsmith wrote: > > Hmm, don't we need to do different things for dot and arrow

[PATCH] D72053: [RFC] Handling implementation limits

rsmith added inline comments. Comment at: clang/include/clang/AST/Decl.h:903 /// declared. -unsigned ScopeDepthOrObjCQuals : NumScopeDepthOrObjCQualsBits; +unsigned ScopeDepthOrObjCQuals : IQ::NumScopeDepthOrObjCQualsBits; These bitfields are assum

[PATCH] D71227: [cuda][hip] Fix function overload resolution in the global initiailizer.

rsmith added inline comments. Comment at: clang/include/clang/Sema/Sema.h:11190 /// combination, based on their host/device attributes. - /// \param Caller function which needs address of \p Callee. - /// nullptr in case of global context. - /// \param Callee

[PATCH] D71758: [Lexer] Allow UCN for dollar symbol '\u0024' in identifiers when using -fdollars-in-identifiers flag.

rsmith added a comment. This seems reasonable to me, and matches GCC 10's behavior; `$` is not in the basic source character set, so encoding it with a UCN should be permitted. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71758/new/ https://revie

[PATCH] D71714: [Sema] Fix -Warray-bounds false negative when casting an out-of-bounds array item

rsmith added a comment. In D71714#1792085 , @efriedma wrote: > To be rigorous, we should perform "pointer" checking for every operation that > performs pointer arithmetic. Then we should perform "lvalue" checking (which > doesn't allow pointers one past

[PATCH] D65591: [AST] Add a flag indicating if any subexpression had errors

rsmith added a comment. Summary of an off-line discussion with Ilya: - The current situation where Clang can do delayed typo correction for C++ but not for C is not a good long-term position. The reason for that situation is that we use a dependent type to represent a type with errors. There ar

[PATCH] D71619: [CLANG] Alignment specifier not applied to anonymous structure or union

rsmith added a comment. Seems fine to me. @aaron.ballman, want to take a look? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71619/new/ https://reviews.llvm.org/D71619 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.ll

[PATCH] D71714: [Sema] Fix -Warray-bounds false negative when casting an out-of-bounds array item

rsmith added a comment. In addition to @riccibruno's comment, I found a couple of other suspicious things nearby (unrelated to the fix in this patch). No need to address them in this patch unless you feel motivated :) Comment at: clang/lib/Sema/SemaChecking.cpp:13384 c

[PATCH] D71619: [CLANG] Alignment specifier not applied to anonymous structure or union

rsmith added inline comments. Comment at: clang/lib/Sema/SemaDecl.cpp:5037 Anon = VarDecl::Create(Context, Owner, DS.getBeginLoc(), Record->getLocation(), /*IdentifierInfo=*/nullptr, Should we apply the attributes to the variabl

[PATCH] D71572: [ItaniumCXXABI] Use linkonce_odr instead of weak_odr for tls wrappers on Windows

rsmith added inline comments. Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:2522 + if (CGM.getTriple().isOSWindows()) +return llvm::GlobalValue::LinkOnceODRLinkage; return llvm::GlobalValue::WeakODRLinkage; I think the thread wrapper should probably be

[PATCH] D71576: [c++20] Add deprecation warnings for the expression forms deprecated by P1120R0.

rsmith marked 3 inline comments as done. rsmith added inline comments. Comment at: clang/lib/AST/Type.cpp:1865-1866 // enumeration type in the sense required here. // C++0x: However, if the underlying type of the enum is fixed, it is // considered complete. if (const

[PATCH] D71576: [c++20] Add deprecation warnings for the expression forms deprecated by P1120R0.

rsmith closed this revision. rsmith marked 4 inline comments as done. rsmith added a comment. Committed as rG4b0029995853fe37d1dc95ef96f46697c743fcad . Comment at: clang/lib/AST/Type.cpp:1865-1866 // enume

[PATCH] D71576: [c++20] Add deprecation warnings for the expression forms deprecated by P1120R0.

rsmith created this revision. rsmith added reviewers: aaron.ballman, rnk. Herald added a project: clang. This covers: - usual arithmetic conversions (comparisons, arithmetic, conditionals) between different enumeration types - usual arithmetic conversions between enums and floating-point types -

[PATCH] D43357: [Concepts] Function trailing requires clauses

rsmith added inline comments. Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:179-180 def err_unexpected_semi : Error<"unexpected ';' before %0">; +def err_expected_primary_got_unary : Error< + "expected primary expression before %0; did you forget parentheses?">;

[PATCH] D71026: Fix LLVM_ENABLE_MODULES=ON + BUILD_SHARED_LIBS=ON build

rsmith added a comment. Please can you describe the problem that this is fixing? Without that, it's hard to know whether this is the appropriate fix. Is there a modules buildbot that's failing, that we could look at the output from? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACT

[PATCH] D71499: Add builtins for aligning and checking alignment of pointers and integers

rsmith added a comment. To me these seem sufficiently useful to be worth having. Please add documentation for them. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2913 +def warn_alignment_builtin_useless : Warning< + "%select{aligning a value|checking whether a

[PATCH] D41910: [Concepts] Constrained partial specializations and function overloads.

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Only a few non-nit comments; if you feel confident addressing those, please feel free to commit after doing so. (If you'd like another round of review, let me know.) Thanks! ===

[PATCH] D71142: [Sema] Validate large bitfields

rsmith added a comment. It seems fine and reasonable to reject this as a violation of an implementation limit. Can we actually support the full range 2^64 bits range, or would it be wiser to pick a smaller limit? (There's at least no point in allowing bit-widths that would mean the size of the

[PATCH] D69778: Make -fmodules-codegen and -fmodules-debuginfo work also with precompiled headers

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. It's a bit weird for this to be controlled by a `-fmodules` flag, but it's only a `-cc1` flag, so I'm OK with that; we can rename it if/when we expose it from the driver. Repository: rC Cl

[PATCH] D70923: Fix comment to more accurately describe C++ language requirements around tail padding.

This revision was automatically updated to reflect the committed changes. Closed by commit rG711c669ae926: Fix comment to more accurately describe C++ language requirements around tail… (authored by rsmith). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.o

<    6   7   8   9   10   11   12   13   14   15   >