[PATCH] D126781: [CodeGen] Keep track info of lazy-emitted symbols in ModuleBuilder

rsmith added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.h:1480-1482 + llvm::SmallPtrSetImpl &getEmittedDeferredDecls() { +return EmittedModuleInitializers; + } This function name doesn't match its implementation. Comment

[PATCH] D125919: Drop qualifiers from return types in C (DR423)

rsmith added a comment. In D125919#3556418 , @aaron.ballman wrote: > In D125919#3556319 , @rjmccall > wrote: > >> Now, the standard has chosen not to talk about `_Atomic` as a qualifier, I >> assume because the

[PATCH] D125517: [Frontend] [Coroutines] Emit error when we found incompatible allocation function in promise_type

rsmith added inline comments. Comment at: clang/lib/Sema/SemaCoroutine.cpp:1312 + + bool PromiseContainNew = [this, &PromiseType]() -> bool { +DeclarationName NewName = Nit, should be `PromiseContainsNew`. Comment at: clang/lib/Sema/SemaCo

[PATCH] D126903: [clang] Add support for __builtin_memset_inline

rsmith added inline comments. Comment at: clang/lib/CodeGen/CGBuilder.h:53 + : CGBuilderBaseTy(C), TypeCache(TypeCache) {} + CGBuilderTy(const CodeGenTypeCache &TypeCache, llvm::LLVMContext &C, + const llvm::ConstantFolder &F, There are a lot o

[PATCH] D118034: [C++20] [Modules] Don't complain about duplicated default template argument across modules

rsmith added inline comments. Comment at: clang/lib/Sema/SemaTemplate.cpp:2651 +Expr *DefaultArgumentY = NTTPY->getDefaultArgument()->IgnoreImpCasts(); +ODRHash X, Y; +X.AddStmt(DefaultArgumentX); ODR hashing is intended to produce a hash that's stabl

[PATCH] D126694: [C++20][Modules] Initial implementation of GMF decl elision.

rsmith added inline comments. Comment at: clang/lib/Sema/Sema.cpp:1150 + continue; +if (!D->isUsed(false) && !D->isReferenced()) + WorkList.push_back(D); rsmith wrote: > ChuanqiXu wrote: > > Should we check for `D->isUsed()` simply? It l

[PATCH] D126694: [C++20][Modules] Initial implementation of GMF decl elision.

rsmith added inline comments. Comment at: clang/lib/Sema/Sema.cpp:1150 + continue; +if (!D->isUsed(false) && !D->isReferenced()) + WorkList.push_back(D); ChuanqiXu wrote: > Should we check for `D->isUsed()` simply? It looks more straight

[PATCH] D126566: [ODRHash][NFC] Add missing 'select' case for `ODRMismatchDecl`.

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. OK, fair enough. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126566/new/ https://reviews.llvm.org/D126566 ___

[PATCH] D126566: [ODRHash][NFC] Add missing 'select' case for `ODRMismatchDecl`.

rsmith added a comment. We perhaps can't or don't want to add test coverage for the "unexpected decl" case here. Can we add coverage for the "function template" case? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126566/new/ https://reviews.llvm.o

[PATCH] D126341: Order implicitly instantiated global variable's initializer by the reverse instantiation order

rsmith added inline comments. Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:563 + } else if (IsInstantiation || getContext().GetGVALinkageForVariable(D) == GVA_DiscardableODR || D->hasAttr()) { rnk wrote: > rsmith wrote: > > rnk wrote:

[PATCH] D125936: [Sema] Relax an assertion in BuildStmtExpr

rsmith added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:15746 ExprResult Sema::BuildStmtExpr(SourceLocation LPLoc, Stmt *SubStmt, SourceLocation RPLoc, unsigned TemplateDepth) { assert(SubStmt && isa(SubStmt) && "Invalid action i

[PATCH] D125936: [Sema] Relax an assertion in BuildStmtExpr

rsmith added inline comments. Comment at: clang/test/CodeGenCXX/asm.cpp:22 + asm("" : : "r"(foo(a)) ); +} + Please add another statement here after the `asm` statement to check that the destructor is called at the end of the `asm` statement, not at the `}`. R

[PATCH] D126341: Order implicitly instantiated global variable's initializer by the reverse instantiation order

rsmith added inline comments. Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:563 + } else if (IsInstantiation || getContext().GetGVALinkageForVariable(D) == GVA_DiscardableODR || D->hasAttr()) { rnk wrote: > @rsmith , if inline global va

[PATCH] D102122: Support warn_unused_result on typedefs

rsmith added inline comments. Comment at: clang/include/clang/Basic/Attr.td:2943 +def WarnUnusedResultClang : InheritableAttr { + let Spellings = [CXX11<"clang", "warn_unused_result">]; + let Subjects = SubjectList<[ObjCMethod, Enum, Record, FunctionLike, TypedefName]>; --

[PATCH] D126341: Order implicitly instantiated global variable's initializer by the reverse instantiation order

rsmith added a comment. > The underlying problem is basically wg21.link/cwg362 which has no concensus > yet. CWG362 has a clear consensus and has been closed with that consensus for 18 years. The consensus, per that issue, is: > We discussed this and agreed that **we really do mean the the ord

[PATCH] D126061: [clang] [WIP] Reject non-declaration C++11 attributes on declarations

rsmith added a comment. I'm very happy with how this patch is looking. Comment at: clang/include/clang/Sema/DeclSpec.h:1926-1928 +assert(llvm::all_of(DeclarationAttrs, [](const ParsedAttr &AL) { + return AL.isStandardAttributeSyntax(); +})); rsmith

[PATCH] D126061: [clang] [WIP] Reject non-declaration C++11 attributes on declarations

rsmith added inline comments. Comment at: clang/include/clang/Sema/DeclSpec.h:1926-1928 +assert(llvm::all_of(DeclarationAttrs, [](const ParsedAttr &AL) { + return AL.isStandardAttributeSyntax(); +})); I think I recall seeing OpenMP pragma attributes

[PATCH] D112374: [clang] Implement ElaboratedType sugaring for types written bare

rsmith added a comment. Herald added a project: All. I've not looked at the test changes in any detail; please let me know if there's anything in there that deserves special attention. Comment at: clang-tools-extra/clang-change-namespace/ChangeNamespace.cpp:567-577 -if (Lo

[PATCH] D111283: [clang] template / auto deduction deduces common sugar

rsmith added inline comments. Herald added a project: All. Comment at: clang/lib/AST/ASTContext.cpp:11551-11552 +if (Unqualified && !Ctx.hasSameType(X, Y)) { + assert(Ctx.hasSameUnqualifiedType(X, Y)); + auto XQuals = X.getCVRQualifiers(), YQuals = Y.getCVRQualifier

[PATCH] D126061: [clang] [WIP] Reject non-declaration C++11 attributes on declarations

rsmith added inline comments. Comment at: clang/lib/Sema/SemaDeclAttr.cpp:9118 + ProcessDeclAttributeOptions Options; + Options.IncludeCXX11Attributes = AL.isCXX11Attribute(); + ProcessDeclAttribute(*this, nullptr, ASDecl, AL, Options); aaron.ballm

[PATCH] D126183: Implement soft reset of the diagnostics engine.

rsmith added a comment. I don't think we can live with the `#define private public` approach, not least because this violates the ODR and might lead to compile failures using modules as a result. As an alternative, how about: - Adding a `friend void DiagnosticTestHelper();` declaration to `Dia

[PATCH] D125773: [Driver] Do not auto-enable header modules with -std=c++20

rsmith added a comment. In D125773#3525127 , @ilya-biryukov wrote: > It looks like this particular change actually breaks standard compatibility > as we also use the same parsing action and don't build a module separately on > `import`. > > [module.i

[PATCH] D126061: [clang] [WIP] Reject non-declaration C++11 attributes on declarations

rsmith added a comment. This direction looks good to me. Thanks! Regarding the possibility of storing the declaration attributes on the `DeclSpec` rather than on the `Declarator`, I wonder if a better choice might be to make the `Declarator` hold a non-owning reference to the attributes, like

[PATCH] D125773: [Driver] Do not auto-enable header modules with -std=c++20

rsmith added a comment. Header modules are part of the C++20 standard (where they are called "header units"), and module maps are an intended way for Clang to provide this functionality in C++20 mode. I don't think turning this off by default in C++20 is the right forward-looking plan; rather,

[PATCH] D111548: [Clang] Add the `annotate_type` attribute

rsmith added inline comments. Comment at: clang/test/SemaCXX/annotate-type.cpp:2 +// RUN: %clang_cc1 %s -std=c++17 -fsyntax-only -verify + +struct S1 { mboehme wrote: > rsmith wrote: > > mboehme wrote: > > > aaron.ballman wrote: > > > > mboehme wrote: > > > > > a

[PATCH] D111548: [Clang] Add the `annotate_type` attribute

rsmith added inline comments. Comment at: clang/test/SemaCXX/annotate-type.cpp:2 +// RUN: %clang_cc1 %s -std=c++17 -fsyntax-only -verify + +struct S1 { mboehme wrote: > aaron.ballman wrote: > > mboehme wrote: > > > aaron.ballman wrote: > > > > mboehme wrote: > >

[PATCH] D124221: Reimplement `__builtin_dump_struct` in Sema.

rsmith added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:433 + + llvm::StringRef getFormatSpecifier(QualType T) { +if (auto *BT = T->getAs()) { yihanaa wrote: > rsmith wrote: > > rsmith wrote: > > > aaron.ballman wrote: > > > > yihanaa wrote

[PATCH] D124221: Reimplement `__builtin_dump_struct` in Sema.

rsmith added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:433 + + llvm::StringRef getFormatSpecifier(QualType T) { +if (auto *BT = T->getAs()) { rsmith wrote: > aaron.ballman wrote: > > yihanaa wrote: > > > I think this is better maintained i

[PATCH] D124221: Reimplement `__builtin_dump_struct` in Sema.

rsmith added inline comments. Comment at: clang/docs/LanguageExtensions.rst:2442 + +This builtin does not return a value. + erichkeane wrote: > I don't know if anyone would be using this value, but I wonder if there is > value to making this a 'sum' of the resul

[PATCH] D124221: Reimplement `__builtin_dump_struct` in Sema.

rsmith updated this revision to Diff 426525. rsmith marked 3 inline comments as done. rsmith added a comment. - Add requested assert. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124221/new/ https://reviews.llvm.org/D124221 Files: clang/docs/La

[PATCH] D124221: Reimplement `__builtin_dump_struct` in Sema.

rsmith updated this revision to Diff 426520. rsmith added a comment. - Use printing policy more, and turn off anonymous tag locations. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124221/new/ https://reviews.llvm.org/D124221 Files: clang/docs/L

[PATCH] D124221: Reimplement `__builtin_dump_struct` in Sema.

rsmith updated this revision to Diff 426517. rsmith marked 5 inline comments as done. rsmith added a comment. - Respond to review comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124221/new/ https://reviews.llvm.org/D124221 Files: clang/d

[PATCH] D124221: Reimplement `__builtin_dump_struct` in Sema.

rsmith updated this revision to Diff 426349. rsmith added a comment. - Update release notes Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124221/new/ https://reviews.llvm.org/D124221 Files: clang/docs/LanguageExtensions.rst clang/docs/ReleaseN

[PATCH] D124221: Reimplement `__builtin_dump_struct` in Sema.

rsmith updated this revision to Diff 426348. rsmith added a comment. - Minor doc correction Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124221/new/ https://reviews.llvm.org/D124221 Files: clang/docs/LanguageExtensions.rst clang/docs/ReleaseN

[PATCH] D124221: Reimplement `__builtin_dump_struct` in Sema.

rsmith updated this revision to Diff 426347. rsmith edited the summary of this revision. rsmith added a comment. - Fix example in documentation: the dump is terminated by a newline. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124221/new/ https://

[PATCH] D124221: Reimplement `__builtin_dump_struct` in Sema.

rsmith added a comment. In D124221#3469251 , @rsmith wrote: > I think we can address most of my concerns with `__builtin_dump_struct` > without a new builtin, by incorporating things from this implementation Done; this patch now reimplements `__builtin_

[PATCH] D124221: Add new builtin __builtin_reflect_struct.

rsmith updated this revision to Diff 426345. rsmith added a comment. - Reimplement __builtin_dump_struct in Sema. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124221/new/ https://reviews.llvm.org/D124221 Files: clang/docs/LanguageExtensions.rst

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

rsmith added inline comments. Comment at: clang/include/clang/AST/TransformTypeTraits.def:27 +TRANSFORM_TYPE_TRAIT_DEF(RemoveVolatile, remove_volatile) +TRANSFORM_TYPE_TRAIT_DEF(EnumUnderlyingType, underlying_type) This file should undef the `TRANSFORM_TYPE_TRAIT

[PATCH] D124221: Add new builtin __builtin_reflect_struct.

rsmith added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:420 +auto *FD = IFD ? IFD->getAnonField() : dyn_cast(D); +if (!FD || (FD->isUnnamedBitfield() || FD->isAnonymousStructOrUnion())) + continue; yihanaa wrote: > erichkeane wrote:

[PATCH] D124221: Add new builtin __builtin_reflect_struct.

rsmith added a comment. In D124221#3471645 , @erichkeane wrote: > In D124221#3470550 , @aaron.ballman > wrote: > >> In D124221#3469251 , @rsmith wrote: >> >>> - Take any

[PATCH] D124258: [C89/C2x] Change the behavior of implicit int diagnostics

rsmith added inline comments. Comment at: clang/docs/ReleaseNotes.rst:168-169 + ``-Wno-error=implicit-int``, or disabled entirely with ``-Wno-implicit-int``. + As of C2x, support for implicit int has been removed, and the warning options + will have no effect. Specifying ``-Wi

[PATCH] D124221: Add new builtin __builtin_reflect_struct.

rsmith added a comment. In D124221#3468706 , @aaron.ballman wrote: > Thank you for looking into this! I've also thought that the > `__builtin_dump_struct` implementation hasn't been quite as robust as it > could be. However, the primary use case for th

[PATCH] D124258: [C89/C2x] Change the behavior of implicit int diagnostics

rsmith added inline comments. Comment at: clang/docs/ReleaseNotes.rst:168-169 + ``-Wno-error=implicit-int``, or disabled entirely with ``-Wno-implicit-int``. + As of C2x, support for implicit int has been removed, and the warning options + will have no effect. Specifying ``-Wi

[PATCH] D124221: Add new builtin __builtin_reflect_struct.

rsmith added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:420 +auto *FD = IFD ? IFD->getAnonField() : dyn_cast(D); +if (!FD || (FD->isUnnamedBitfield() || FD->isAnonymousStructOrUnion())) + continue; rsmith wrote: > erichkeane wrote:

[PATCH] D124221: Add new builtin __builtin_reflect_struct.

rsmith added inline comments. Comment at: clang/docs/LanguageExtensions.rst:2434 + + bool print(int arg1, int arg2, std::initializer_list fields) { + for (auto f : fields) { erichkeane wrote: > rsmith wrote: > > erichkeane wrote: > > > I'm curious as to

[PATCH] D124221: Add new builtin __builtin_reflect_struct.

rsmith added a comment. In D124221#3467467 , @erichkeane wrote: > I generally just question the usefulness of this. Despite its shortcomings, > the 'dump struct' has the capability of runtime introspection into a type, > whereas this seems to require

[PATCH] D124221: Add new builtin __builtin_reflect_struct.

rsmith updated this revision to Diff 424379. rsmith added a comment. - Fix some code that was making incorrect assumptions that PseudoObjectExpr is only used for ObjC properties. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124221/new/ https://re

[PATCH] D124221: Add new builtin __builtin_reflect_struct.

rsmith updated this revision to Diff 424350. rsmith added a comment. - Some improvements. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124221/new/ https://reviews.llvm.org/D124221 Files: clang/docs/LanguageExtensions.rst clang/docs/ReleaseNot

[PATCH] D124221: Add new builtin __builtin_reflect_struct.

rsmith added a comment. I'm not entirely sure whether I want to pursue this -- I'd prefer to have only a single mechanism that works well in both C and C++, rather than this (which is flexible but not really usable in C) and `__builtin_dump_struct` (which is extremely inflexible but works well

[PATCH] D124221: Add new builtin __builtin_reflect_struct.

rsmith created this revision. rsmith added reviewers: aaron.ballman, erichkeane, yihanaa. Herald added a project: All. rsmith requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. This builtin, like __builtin_dump_struct, allows for limited intro

[PATCH] D122920: [Clang][CodeGen]Fix __builtin_dump_struct missing record type field name

rsmith added inline comments. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2046-2048 static llvm::Value *dumpRecord(CodeGenFunction &CGF, QualType RType, LValue RecordLV, CharUnits Align, + bool dumpTypeName, llvm::Fun

[PATCH] D119136: [clang] Implement Change scope of lambda trailing-return-type

rsmith added a comment. In D119136#3463888 , @cor3ntin wrote: > @rsmith @aaron.ballman Do you think it's worth resubmitting this patch with a > look ahead of the `mutable` keyword (this seems to me a better strategy than > other flimsy workaround sugges

[PATCH] D119136: [clang] Implement Change scope of lambda trailing-return-type

rsmith added a comment. In D119136#3463888 , @cor3ntin wrote: > @rsmith @aaron.ballman Do you think it's worth resubmitting this patch with a > look ahead of the `mutable` keyword (this seems to me a better strategy than > other flimsy workaround sugges

[PATCH] D123345: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins.

rsmith added a comment. In D123345#3463540 , @thakis wrote: > I think your revert to fix the aix CI broke CI everywhere: > http://45.33.8.238/linux/74239/step_7.txt > > Please take a look and reland for now if it takes a while to fix. Relanded with a wo

[PATCH] D123345: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins.

rsmith added a comment. In D123345#3463224 , @daltenty wrote: > In D123345#3460639 , @Jake-Egan > wrote: > >> Hi, unfortunately there's a build failure on AIX: >> https://lab.llvm.org/buildbot/#/builders/214/bui

[PATCH] D123909: [Clang] Use of decltype(capture) in parameter-declaration-clause

rsmith added a comment. In D123909#3462560 , @aaron.ballman wrote: > This looks like a true positive to me, at least for the moment (Core is still > trying to decide what to do about CWG2569 which may relax some restrictions > in this area). > > @MaskR

[PATCH] D122983: [C11/C2x] Change the behavior of the implicit function declaration warning

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. LGTM; two non-blocking comments and a question. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:420-422 +def ext_implicit_function_decl_c99 : ExtWarn< + "implic

[PATCH] D123955: [C2x] Disallow functions without prototypes/functions with identifier lists

rsmith accepted this revision. rsmith added a comment. LGTM. Some comments for potential improvements, but I'd be OK with this landing as-is if you don't find any of them sufficiently motivating :) Comment at: clang/lib/Parse/ParseDecl.cpp:6664- +// OpenCL disallows va

[PATCH] D123955: [C2x] Disallow functions without prototypes/functions with identifier lists

rsmith added inline comments. Comment at: clang/include/clang/Basic/LangOptions.def:124 BENIGN_LANGOPT(ImplicitInt, 1, 0, "C89 implicit 'int'") +LANGOPT(StrictPrototypes , 1, 0, "require function types to have a prototype") LANGOPT(Digraphs , 1, 0, "digraphs") ---

[PATCH] D122663: Mark identifier prefixes as substitutable

rsmith added inline comments. Comment at: clang/lib/AST/ItaniumMangle.cpp:6031 +bool CXXNameMangler::mangleSubstitution(NestedNameSpecifier *NNS) { + NNS = Context.getASTContext().getCanonicalNestedNameSpecifier(NNS); + return mangleSubstitution(reinterpret_cast(NNS));

[PATCH] D123345: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins.

rsmith added a comment. In D123345#3457109 , @rsmith wrote: > I'm working on a fix. I'll revert if it takes me more than a few minutes. If > you'd prefer to not wait, please feel free to revert it yourself. Thanks for your patience, should be fixed in

[PATCH] D123345: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins.

rsmith added a comment. In D123345#3456964 , @paulkirth wrote: > Hi, this is also breaking Fuchsia's clang CI builders > (https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8816531831869640417/overview). > If this will be

[PATCH] D123345: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins.

This revision was automatically updated to reflect the committed changes. Closed by commit rGb27430f9f46b: Treat `std::move`, `forward`, etc. as builtins. (authored by rsmith). Changed prior to commit: https://reviews.llvm.org/D123345?vs=423165&id=423312#toc Repository: rG LLVM Github Monore

[PATCH] D123345: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins.

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG64c045e25b84: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins. (authored by rsmith). Changed prior to commit: https://reviews.llv

[PATCH] D123345: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins.

rsmith added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:6577-6578 +// FIXME: This should also be in -Wc++23-compat once we have it. +def warn_use_of_unaddressable_function : Warning< + "taking address of non-addressable standard library functio

[PATCH] D123345: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins.

rsmith updated this revision to Diff 423164. rsmith marked 3 inline comments as done. rsmith added a comment. - Address review comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123345/new/ https://reviews.llvm.org/D123345 Files: clang/docs

[PATCH] D123642: [clang codegen] Assume arguments of __atomic_* are aligned.

rsmith added a comment. In D123642#3450129 , @xbolva00 wrote: > Do you have any comments related to this issue by gcc devs that this is a > "known" bug? https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87237#c1, from someone in the GCC MAINTAINERS file, su

[PATCH] D122983: [C11/C2x] Change the behavior of the implicit function declaration warning

rsmith added a comment. In D122983#3454508 , @aaron.ballman wrote: > In D122983#3454494 , @jyknight > wrote: > >> In D122983#3454406 , >> @aaron.ballman wrote: >> >>> O

[PATCH] D122983: [C11/C2x] Change the behavior of the implicit function declaration warning

rsmith added a comment. In D122983#3451920 , @erichkeane wrote: > I think Aaron's approach provides a proper 'depreciation' period in our > compiler, as best as we have the ability to do. We've been warning by default for a decade that this code is not

[PATCH] D123345: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins.

rsmith added a comment. In D123345#3452496 , @aaron.ballman wrote: > Do you have ideas on how we can improve the debugging checkpoint behavior (if > at all)? I think we just live with it, like we do for other builtin functions. (There might be things

[PATCH] D119136: [clang] Implement Change scope of lambda trailing-return-type

rsmith added a comment. Hey @cor3ntin, sorry, I reverted this again. It's breaking thread-safety annotations on lambdas in a pretty severe way: Mutex m; auto lambda = [=]() EXCLUSIVE_LOCKS_REQUIRED(m) {...} doesn't compile any more. Presumably we should be parsing the attribute in the enc

[PATCH] D122983: [C11/C2x] Change the behavior of the implicit function declaration warning

rsmith added a comment. It seems surprising to me for the diagnostic to change from warn-by-default to error-by-default when changing from C99 to C11, given that the language rule did not change between C99 and C11 (as a Clang user, when changing my `-std=` flag, I don't want other changes to c

[PATCH] D113545: [C++20] [Module] Support reachable definition initially/partially

rsmith added a comment. Thanks for working on this! Comment at: clang/include/clang/AST/DeclBase.h:228 +/// This declaration has an owninig module, and is visible to lookups +/// that occurs within that module. And it is reachable to other module =

[PATCH] D123648: Restrict lvalue-to-rvalue conversions in CGExprConstant.

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123648/new/ https://reviews.llvm.org/D123648 ___

[PATCH] D123345: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins.

rsmith added inline comments. Comment at: clang/docs/CommandGuide/clang.rst:264 + +.. option:: -fno-builtin-std- + xbolva00 wrote: > -fno-builtin=xxx,yyy,zzz > > wdyt? Seems reasonable to me to treat `-fno-builtin-` and `-fno-builtin=` as equivalent, and I cert

[PATCH] D123345: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins.

rsmith updated this revision to Diff 422366. rsmith added a comment. - Switch to a different example with underscores. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123345/new/ https://reviews.llvm.org/D123345 Files: clang/docs/CommandGuide/clan

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

rsmith reopened this revision. rsmith added a comment. This revision is now accepted and ready to land. Herald added a project: All. @dexonsmith @arphaman What do we need to do to get this re-landed? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D676

[PATCH] D123345: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins.

rsmith added a comment. In D123345#3440935 , @aaron.ballman wrote: > Any chance you'd be interested in submitting this patch to > http://llvm-compile-time-tracker.com/ (instructions at > http://llvm-compile-time-tracker.com/about.php) so we can have an

[PATCH] D123345: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins.

rsmith updated this revision to Diff 422311. rsmith added a comment. - Document `-fno-builtin-std-foo`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123345/new/ https://reviews.llvm.org/D123345 Files: clang/docs/CommandGuide/clang.rst clang/d

[PATCH] D123345: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins.

rsmith updated this revision to Diff 422305. rsmith added a comment. - Add support for -fno-builtin, -ffreestanding, -fno-builtin-std-move. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123345/new/ https://reviews.llvm.org/D123345 Files: clang/d

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

rsmith added a comment. The patch description doesn't match the patch: we have `__remove_cv` in the description but `__remove_cv_qualifiers` in the patch. It seems unfortunate to me to use a long-term bad name for our trait to work around a (likely) short-term problem with a specific version o

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

rsmith added inline comments. Comment at: clang/lib/AST/ItaniumMangle.cpp:3957 switch (T->getUTTKind()) { case UnaryTransformType::EnumUnderlyingType: I'd prefer that you restructure this to first compute a `StringRef` corresponding to the name of

[PATCH] D123345: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins.

rsmith created this revision. rsmith added reviewers: aaron.ballman, rnk. Herald added a project: All. rsmith requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. We still require these functions to be declared before they can be used, but don't

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

rsmith added inline comments. Comment at: clang/lib/AST/ItaniumMangle.cpp:3928 break; + case UnaryTransformType::AddConst: +Out << "2ac"; cjdb wrote: > aaron.ballman wrote: > > Are these the suggested manglings from the Itanium mangling docu

[PATCH] D122248: [clang][CodeGen]Fix clang crash and add bitfield support in __builtin_dump_struct

rsmith added inline comments. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2113-2117 if (CanonicalType->isRecordType()) { - TmpRes = dumpRecord(CGF, CanonicalType, FieldPtr, Align, Func, Lvl + 1); + TmpRes = dumpRecord(CGF, CanonicalType, FieldLV, Align, Func, Lvl +

[PATCH] D122822: [Clang][CodeGen]Add constant array support for __builtin_dump_sturct

rsmith added a comment. One thing we could do would be to build a descriptor for the type (as data) -- a type name string, a type "kind", plus (for a record type) a list of (field type descriptor, field name, offset) tuples -- and pass it to a user-specified function. We could provide a library

[PATCH] D122822: [Clang][CodeGen]Add constant array support for __builtin_dump_sturct

rsmith added subscribers: rjmccall, rsmith. rsmith added a comment. I'm concerned about the direction of this patch given @rjmccall's comments on https://reviews.llvm.org/D112626 -- presumably the way we'd want to address those comments would be to convert a `__builtin_dump_struct(a, f)` call in

[PATCH] D121765: [CUDA][HIP] Fix hostness check with -fopenmp

rsmith accepted this revision. rsmith added inline comments. This revision is now accepted and ready to land. Comment at: clang/include/clang/Sema/Sema.h:3325-3330 + /// getCurFunctionDecl - If parsing a lambda, then return the lambda + /// declaration if \p AllowLambda is true

[PATCH] D114425: [clang] Add __builtin_bswap128

rsmith added inline comments. Comment at: clang/lib/AST/ASTContext.cpp:11071-11072 -QualType ElementType = DecodeTypeFromStr(Str, Context, Error, - RequiresICE, false); +QualType ElementType = +DecodeTypeFromStr(Str, C

[PATCH] D118525: [modules] Merge ObjC interface ivars with anonymous types.

rsmith added a comment. The modules side of this looks good to me, and logically changing the lexical decl context in an interface to be that interface makes a lot of sense, but I agree with @rjmccall that it's hard to predict what the consequences of that change might be. Can you also test thi

[PATCH] D99134: Lambdas are not necessarily locals. This resolves DR48250.

rsmith accepted this revision. rsmith added a comment. Looks good. Do you need someone to land this for you? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99134/new/ https://reviews.llvm.org/D99134 ___ c

[PATCH] D112916: Confusable identifiers detection

rsmith added a comment. In D112916#3290365 , @serge-sans-paille wrote: > @rsmith : once the licensing issue has been fixed, can we merge that patch or > do you have any other thought? I have no concerns once the licensing question is resolved and the o

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

rsmith added a comment. In D119778#3327161 , @Quuxplusone wrote: > Apply @rsmith's suggested approach. Thanks, the structure of the change looks good to me. Looking through the test changes, I'm a bit torn by the value proposition of this note. Some o

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

rsmith 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 lambda, we

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

rsmith added a comment. In D119778#3326285 , @Quuxplusone wrote: > 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.

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

rsmith added inline comments. Comment at: clang/lib/Sema/SemaStmt.cpp:3804-3809 +if (DAR != DAR_Succeeded) { + if (OrigResultType.getBeginLoc().isValid()) +Diag(OrigResultType.getBeginLoc(), diag::note_deducing_return_type_for) +<< FD << OrigResultTyp

[PATCH] D99134: Lambdas are not necessarily locals. This resolves DR48250.

rsmith accepted this revision. rsmith added a comment. LGTM. Please also add something like this as a unit test: In D99134#2643441 , @rsmith wrote: > template int x = [](auto){ return T(); }.operator()(T()); > int y = x; Repository: rG LLVM Githu

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

rsmith added a comment. I support this revert. `malloc`, `operator new`, and `operator new[]` (by the latter two I mean the usual global allocation functions, not user-provided ones) follow these rules: - `malloc` always returns storage whose alignment is at least the largest fundamental align

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

This revision was automatically updated to reflect the committed changes. Closed by commit rG19aa2db023c0: [clang] Mark `trivial_abi` types as "trivially relocatable". (authored by devin.jeanpierre, committed by rsmith). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://

[PATCH] D118511: Add a warning for not packing non-POD members in packed structs

rsmith accepted this revision. rsmith added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:2029-2036 // The align if the field is not packed. This is to check if the attribute // was unnecessary (-Wpacked)

<    1   2   3   4   5   6   7   8   9   10   >