[PATCH] D117616: GCC ABI Compatibility: Preserve alignment of non-pod members in packed structs

rsmith added inline comments. Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1891 + llvm::Triple Target = Context.getTargetInfo().getTriple(); + bool FieldPacked = (Packed && (!FieldClass || FieldClass->isPOD() || +

[PATCH] D103395: PR45879: Keep evaluated expression in LValue object

rsmith added a comment. Alternative fix landed in rG30baa5d2a450d5e302d8cba3fc7a26a59d4b7ae1 . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103395/new/

[PATCH] D103395: PR45879: Keep evaluated expression in LValue object

rsmith added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:1549 SubobjectDesignator Designator; +const Expr *LExpr = nullptr; bool IsNullPtr : 1; I don't think this is an appropriate approach. A source expression is fundamentally not

[PATCH] D115610: [C++20] [Modules] Don't create multiple global module fragment

rsmith added a comment. Thanks, this looks functionally good to me. I'm happy for this to land with the `Sema` member renamed. Comment at: clang/lib/Sema/SemaModule.cpp:727 +ModuleMap = PP.getHeaderSearchInfo().getModuleMap(); +GlobalModuleFragmentCache =

[PATCH] D117616: GCC ABI Compatibility: Preserve alignment of non-pod members in packed structs

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. LGTM Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1891 + llvm::Triple Target = Context.getTargetInfo().getTriple(); + bool FieldPacked = (Packed && (!FieldClass ||

[PATCH] D117616: GCC ABI Compatibility: Preserve alignment of non-pod members in packed structs

rsmith added inline comments. Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1890 - bool FieldPacked = Packed || D->hasAttr(); + llvm::Triple Target = Context.getTargetInfo().getTriple(); + bool FieldPacked = (Packed && (!FieldClass || FieldClass->isPOD() ||

[PATCH] D117616: GCC ABI Compatibility: Preserve alignment of non-pod members in packed structs

rsmith added inline comments. Comment at: clang/docs/ReleaseNotes.rst:239 +- gcc doesn't pack non-packed non-pod members in packed structs. Clang + historically did perform such packing. Clang now matches the gcc behavior "non-packed" here seems a little

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

rsmith 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.

[PATCH] D116983: Clone constructor template parameters when creating deduction guide

rsmith added inline comments. Comment at: clang/lib/Sema/SemaTemplate.cpp:2174 + Args.addOuterTemplateArguments(SubstArgs); + Args.addOuterRetainedLevel(); + NamedDecl *NewParam = transformTemplateParameter(Param, Args); aeubanks wrote: > rsmith

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

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/D114732/new/ https://reviews.llvm.org/D114732

[PATCH] D115031: [AST] Print NTTP args as string-literals when possible

rsmith added inline comments. Comment at: clang/include/clang/AST/PrettyPrinter.h:288 + /// template parameters, no matter how many elements there are. + unsigned EntireContentsOfLargeArray : 1; + lichray wrote: > rsmith wrote: > > It looks like nothing is

[PATCH] D116833: [clang] Introduce support for disabling warnings in system macros

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Thanks, this looks nice. I think we'll need to think carefully before changing the default here. It seems like the choice here would depend on what token the location of the diagnostic

[PATCH] D113838: Sema: Don't erroneously reject `void{}`

rsmith added a comment. This is CWG issue 2351. Please add a corresponding test to `tests/CXX/drs/dr23xx.cpp`. Comment at: clang/lib/Sema/SemaInit.cpp:1311-1319 + } else if (DeclType->isVoidType()) { +// [expr.type.conv]p2: Otherwise, if the type is cv void and the +

[PATCH] D114908: [clang] Don't call inheritDefaultTemplateArguments() on CXXDeductionGuideDecl's template parameters

rsmith added inline comments. Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:3784-3790 + if (auto *TD = dyn_cast(D)) { +// CXXDeductionGuideDecls reference the class template parameters so we need +// to make sure not to call this twice on the same template

[PATCH] D116833: [clang][Sema] Disable -Wc++20-designator in system macros

rsmith added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:7161 +!DiagnosedNestedDesignator && !DiagnosedMixedDesignator && +!getSourceManager().isInSystemMacro(FirstDesignator)) { Diag(FirstDesignator, getLangOpts().CPlusPlus20

[PATCH] D115031: [AST] Print NTTP args as string-literals when possible

rsmith added inline comments. Comment at: clang/include/clang/AST/PrettyPrinter.h:288 + /// template parameters, no matter how many elements there are. + unsigned EntireContentsOfLargeArray : 1; + It looks like nothing is setting this to `true` yet, so that

[PATCH] D114206: [Clang][ScanDeps] Use the virtual path for module maps

rsmith added inline comments. Comment at: clang/test/ClangScanDeps/modulemap-via-vfs.m:5-6 +// RUN: sed -e "s|DIR|%/t.dir|g" %t.dir/build/vfs.yaml.in > %t.dir/build/vfs.yaml +// RUN: clang-scan-deps -compilation-database %t.dir/build/compile-commands.json -j 1 -format

[PATCH] D115625: [clang-format] add support for cppm files

rsmith added a comment. FWIW, Clang also supports `.ccm`, `.cxxm`, and `.c++m`. See https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/Types.cpp#L273 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115625/new/ https://reviews.llvm.org/D115625

[PATCH] D110215: [C++2a] [Module] Support extern C/C++ semantics

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Thanks, this looks great. A couple of minor suggestions. Comment at: clang/include/clang/Lex/ModuleMap.h:542-543 + /// unit's Module until later, because we don't know what

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

rsmith added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:3211-3215 +The compiler will pass and return a trivially relocatable type using the C ABI +for the underlying type, even when the type would otherwise be considered +non-trivially-relocatable. If a

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

rsmith added inline comments. Comment at: clang/lib/AST/Type.cpp:2500 +return BaseElementType.isTriviallyCopyableType(Context) && + !BaseElementType.isDestructedType(); + } devin.jeanpierre wrote: > rsmith wrote: > > You can simplify this a little

[PATCH] D113620: Skip exception cleanups when the innermost scope is EHTerminateScope.

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. I think it's generally better to not run cleanups on the way to a `terminate` call, not only for code size but also for debuggability. This change seems like an improvement to me.

[PATCH] D115031: [AST] Print NTTP args as string-literals when possible

rsmith added inline comments. Comment at: clang/lib/AST/APValue.cpp:637-639 + // Nothing we can do about a sequence that is not null-terminated + if (!Data[--Size].getInt().isZero()) +return false; lichray wrote: > rsmith wrote: > > We should drop all

[PATCH] D115031: [AST] Print NTTP args as string-literals when possible

rsmith added inline comments. Comment at: clang/lib/AST/APValue.cpp:628-629 +static bool TryPrintAsStringLiteral(raw_ostream , const ArrayType *ATy, +const APValue *Data, size_t Size) { + if (Size == 0) Is there anything

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

rsmith added a comment. In D114732#3159247 , @Quuxplusone wrote: > - D50119 has more extensive tests, and has > been live on https://p1144.godbolt.org/z/axx9Wj3r5 for a couple years now; if > the consensus is that

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

rsmith added a comment. Generally this looks good. In addition to the `ExtWarn` for the `decltype(auto)` case, I think we're also missing a `-Wc++20-compat` warning for the feature as a whole. Comment at:

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

rsmith added inline comments. Comment at: clang/lib/Parse/ParseDeclCXX.cpp:1032 + // the typename-specifier in a function-style cast expression may + // be 'auto' since C++2b Diag(Tok.getLocation(), Quuxplusone wrote: > rsmith wrote: > > Nice

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

rsmith added a comment. I also wonder whether we should accept `decltype(auto)(x)` as an extension, but we can discuss that separately. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113393/new/ https://reviews.llvm.org/D113393

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

rsmith added a comment. Thanks, this generally looks great. It looks like we'll need some additional work on disambiguation to handle cases like: struct A { int n; } a; void f() { auto()->n = 0; } I think that's valid, but right now we misparse it as a declaration of a variable ``. (This

[PATCH] D77598: Integral template argument suffix and cast printing

rsmith added inline comments. Comment at: clang/lib/AST/DeclPrinter.cpp:1101 Out << ", "; -Args[I].print(Policy, Out); +if (TemplOverloaded || !Params) + Args[I].print(Policy, Out, /*IncludeType*/ true); dblaikie wrote: > dblaikie wrote: > >

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Awesome! Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:559 + // Get the resolved template arguments from the canonical type. + // FIXME: Handle the as-written

[PATCH] D113517: Correct handling of the 'throw()' exception specifier in C++17.

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. In D113517#3121455 , @jyknight wrote: > This change allows those future optimizations to apply to throw() as well, in > C++17 mode, which is the

[PATCH] D113517: Correct handling of the 'throw()' exception specifier in C++17.

rsmith added a comment. What's the motivation for this change? I believe the current behavior is still conforming: `set_unexpected` is no longer (officially) part of the standard library (though it still exists as a zombie name), and the default `unexpected` handler calls `terminate`, so

[PATCH] D112663: [clang-repl] Allow Interpreter::getSymbolAddress to take a mangled name.

rsmith accepted this revision. rsmith added inline comments. This revision is now accepted and ready to land. Comment at: clang/include/clang/Interpreter/Interpreter.h:72 + + ///\returns the \c JITTargetAddress of a \c GlobalDecl. This interface uses + /// the CodeGenModule's

[PATCH] D113391: [Modules] Don't support clang module and c++20 module at the same time

rsmith requested changes to this revision. rsmith added a comment. This revision now requires changes to proceed. The ideal situation here would be to have a single semantic model that is a superset of the various combinations of behaviors we want. Further entrenching Clang modules and C++20

[PATCH] D112914: Misleading identifier detection

rsmith accepted this revision. rsmith added a comment. In D112914#3102728 , @carlosgalvezp wrote: > I don't know how to remove the "Requested changes" from here so I'll just > remove myself from reviewer. Marking the change as accepted should

[PATCH] D112914: Misleading identifier detection

rsmith added inline comments. Comment at: clang-tools-extra/clang-tidy/misc/MisleadingIdentifier.h:2 +//===--- MisleadingIdentifierCheck.h - clang-tidy *- C++ +//-*-===// +// This needs fixing too. CHANGES SINCE LAST ACTION

[PATCH] D112913: Misleading bidirectional detection

rsmith added inline comments. Comment at: clang-tools-extra/clang-tidy/misc/MisleadingBidirectional.cpp:59 +// If conversion fails, utf-8 is designed so that we can just try next char. +if (Result != llvm::conversionOK) { + ++CurPtr; Is there a

[PATCH] D112453: [Sema] When dereferencing a pointer of dependent type, infer the result type.

rsmith added a comment. I think the idea of this change is OK. The key is that the dereference expression will still be type-dependent, even if we happen to actually know its type. (For an `Expr` that `isTypeDependent()`, the type produced by `getType()` isn't something the standard knows or

[PATCH] D112914: Misleading identifier detection

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Looks good as far as it goes (though I've not checked your classification functions are correct). We should have some detection for RTL characters in string literals and comments too, at

[PATCH] D112913: Misleading bidirectional detection

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Nits and a suggested approach for invalid code sequences that's probably important to handle better. Please fix the clang-tidy findings too. Otherwise, LGTM. Comment at:

[PATCH] D112916: Confusable identifiers detection

rsmith added inline comments. Comment at: clang-tools-extra/clang-tidy/misc/CMakeLists.txt:9 +add_custom_command( +OUTPUT confusables.h +COMMAND make_confusable_table ${CMAKE_CURRENT_SOURCE_DIR}/ConfusableTable/confusables.txt ${CMAKE_CURRENT_BINARY_DIR}/confusables.h

[PATCH] D112883: [bazel] Re-introduce `copts` hacks for lib/AST includes.

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. :( LGTM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112883/new/ https://reviews.llvm.org/D112883 ___ cfe-commits mailing list

[PATCH] D112101: [AST] Fix the EndLoc calculation for ObjCObjectPointer

rsmith added a comment. In D112101#3097387 , @rsmith wrote: > I think this is still not quite right Fixed in rG68ffcd521347e3c8b97ca233239d503beff239f4 . Repository: rG LLVM

[PATCH] D112101: [AST] Fix the EndLoc calculation for ObjCObjectPointer

rsmith added a comment. I think this is still not quite right: if the `ObjCObjectPointerType` has a valid star location, we should use that. We should only be falling back on the inner location's end when there is no star. For example: @interface X @end; using ObjectPointer = X*; ...

[PATCH] D112663: [clang-repl] Allow Interpreter::getSymbolAddress to take a mangled name.

rsmith added inline comments. Comment at: clang/include/clang/Interpreter/Interpreter.h:70 llvm::Expected - getSymbolAddress(llvm::StringRef UnmangledName) const; + getSymbolAddress(llvm::StringRef Name, bool IsMangled = false) const; }; I find this flag

[PATCH] D112659: Add a limit to __make_integer_seq builtin.

rsmith added a comment. I wonder if more generally we should have a limit on either the size of a pack or the number of template arguments in a template specialization, rather than limiting only `__make_integer_seq`? I think a hand-rolled `make_integer_sequence` implementation should also be

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

rsmith added inline comments. Herald added a subscriber: carlosgalvezp. Comment at: clang/include/clang/AST/Type.h:4968 QualType getDeducedType() const { -return !isCanonicalUnqualified() ? getCanonicalTypeInternal() : QualType(); +return DeducedAsType.isNull() ?

[PATCH] D109496: [clang] deprecate frelaxed-template-template-args, make it on by default

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Thanks, let's land this and see how it goes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109496/new/ https://reviews.llvm.org/D109496

[PATCH] D101192: Add support for #elifdef and #elifndef

rsmith added a comment. > In this version of the patch, I am supporting the feature in all C and C++ > modes. This seems like more useful functionality than diagnosing an unknown > preprocessor directive in older language modes, and is a conforming extension > in those modes anyway as use of

[PATCH] D110287: [modules] While merging ObjCInterfaceDecl definitions, merge them as decl contexts too.

rsmith accepted this revision. rsmith added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:1181-1184 + if (DD.Definition != NewDD.Definition) { +Reader.MergedDeclContexts.insert( +

[PATCH] D110287: [modules] While merging ObjCInterfaceDecl definitions, merge them as decl contexts too.

rsmith added inline comments. Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:1181-1184 + if (DD.Definition != NewDD.Definition) { +Reader.MergedDeclContexts.insert( +std::make_pair(NewDD.Definition, DD.Definition)); + } A couple of other

[PATCH] D112089: consteval if does not form a discarded statement

rsmith accepted this revision. rsmith added inline comments. Comment at: clang/lib/Parse/ParseStmt.cpp:1445-1450 Sema::ExpressionEvaluationContext Context = Sema::ExpressionEvaluationContext::DiscardedStatement; -if (NotLocation.isInvalid() && IsConsteval) +

[PATCH] D112089: consteval if does not form a discarded statement

rsmith added inline comments. Comment at: clang/lib/Parse/ParseStmt.cpp:1445-1450 Sema::ExpressionEvaluationContext Context = Sema::ExpressionEvaluationContext::DiscardedStatement; -if (NotLocation.isInvalid() && IsConsteval) +if (NotLocation.isInvalid() &&

[PATCH] D111543: [clang][modules] Stop creating `IdentifierInfo` for names of explicit modules

rsmith added a comment. I've stepped through this in a debugger. The problem is that the `CompilerInstance` is setting up an `ASTReaderListener` that is informed whenever a module is loaded, and that listener is called when the AST file has been added to the module manager but hasn't had its

[PATCH] D111543: [clang][modules] Stop creating `IdentifierInfo` for names of explicit modules

rsmith added a comment. In D111543#3067619 , @rsmith wrote: > In D111543#3066686 , @jansvoboda11 > wrote: > >> My understanding is that `Preprocessor::getIdentifierInfo` may trigger AST >> deserialization. The

[PATCH] D111543: [clang][modules] Stop creating `IdentifierInfo` for names of explicit modules

rsmith added a comment. In D111543#3066686 , @jansvoboda11 wrote: > My understanding is that `Preprocessor::getIdentifierInfo` may trigger AST > deserialization. The problem here seems to be that we're calling > `getIdentifierInfo` before the

[PATCH] D111817: Fix a rejects-valid with consteval on overloaded operators

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Looks good to me. Comment at: clang/lib/Sema/SemaOverload.cpp:14290 return MaybeBindToTemporary(call); } aaron.ballman wrote: > erichkeane wrote:

[PATCH] D111543: [clang][modules] Stop creating `IdentifierInfo` for names of explicit modules

rsmith added a comment. I'm concerned that this is adding complexity to paper over a bug elsewhere. Creating an `IdentifierInfo` should never cause unrelated uses of that identifier to change their meaning or behavior. My guess is that there's a bug in how we resolve or merge identifier

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

rsmith added inline comments. Comment at: clang/test/SemaCXX/sugared-auto.cpp:146 +return a; + return N(); // expected-error {{but deduced as 'SARS (*)() throw(Man, Vibrio)' (aka 'void (*)() throw(Man, Vibrio)')}} +} rsmith wrote: > Why don't we get

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

rsmith added a comment. Do you have examples showing what this does in practice for errors in real-world code? I'm concerned that stripping back to the common type sugar may produce an unintuitive result in some cases, although it certainly does seem at least theoretically nice to use a

[PATCH] D110215: [C++2a] [Module] Support extern C/C++ semantics

rsmith added a comment. Instead of moving functions and classes/enums into the global module after creating them, it would be better to push a module scope for the global module when entering a C or C++ language linkage specification and pop the module scope when leaving the linkage

[PATCH] D111175: [Clang] Extend init-statement to allow alias-declaration

rsmith added inline comments. Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:553 +def err_expected_alias_after_using_in_init_statement : Error< + "expected alias declaration after using in init statement">; +def ext_alias_in_init_statement : ExtWarn<

[PATCH] D110484: [clang-repl] Allow loading of plugins in clang-repl.

rsmith added a comment. LGTM modulo existing comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110484/new/ https://reviews.llvm.org/D110484 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D110780: [clang] do not emit note for bad conversion when destination type qualifiers don't compatibly include source type qualifiers

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Thanks! Would it be worth also including a testcase where the from and to types are different classes but the qualifications are incompatible, to ensure we're handling that case too?

[PATCH] D109496: [clang] deprecate frelaxed-template-template-args, make it on by default

rsmith added inline comments. Comment at: clang/www/cxx_status.html:817 https://wg21.link/p0522r0;>P0522R0 - Partial (10) + Clang 14 We should list this as implemented in Clang 4, with a footnote saying that until Clang 14 you need to

[PATCH] D110726: [clang] NFC: remove duplicated code around type constraint and templ arg subst

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

[PATCH] D110727: [clang] don't instantiate templates with injected arguments

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. I think it's almost always true that we don't substitute into a template unless we've already substituted into all enclosing templates. The only exception I can think of is alias templates,

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

rsmith added a comment. Thanks, this is very exciting, and the diagnostic changes in the test suite look great! Comment at: clang-tools-extra/clangd/unittests/ASTTests.cpp:180 )cpp", - // FIXME: it'd be nice if this resolved to the alias instead -

[PATCH] D85390: [Clang] Add note for bad conversion error when expression is of forward-declared type

rsmith added a comment. Please see llvm.org/PR52014 -- this patch is not properly handling cv-qualifiers. We should not produce the new note if the destination type has qualifiers that the source type does not possess, because in that case inheritance makes no difference to whether the

[PATCH] D109800: [clang] don't mark as Elidable CXXConstruct expressions used in NRVO

rsmith added a comment. There might be some tooling out there that cares about `CXXConstructExpr`s that might be elided due to NRVO being marked as elidable. But it's probably OK to lose that marking if nothing in-tree is relying on it, and use `isElidable` only for copy elision from rvalues

[PATCH] D106876: Remove non-affecting module maps from PCM files.

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Please apply the lint suggestions; otherwise, looks good. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D106876/new/ https://reviews.llvm.org/D106876

[PATCH] D104344: [modules] Track how headers are included by different submodules.

rsmith added a comment. I wonder if perhaps we're tracking this state in the wrong way. The "has been included" information for `#pragma once` / `#import` should behave exactly like macro definition visibility: it should be reset whenever we enter a new "clean slate" state and should be saved

[PATCH] D108905: [ItaniumCXXABI] Make __cxa_end_catch calls unconditionally nounwind

rsmith added a comment. No decision as yet, but so far it looks very likely that we'll settle on the rule that exceptions cannot have potentially-throwing destructors, and that we should reject `throw`s of such types. I don't think that should be applied retroactively to C++98, though, because

[PATCH] D107049: [clang-repl] Re-implement clang-interpreter as a test case.

rsmith added inline comments. Comment at: clang/unittests/Interpreter/ExceptionTests/InterpreterExceptionTest.cpp:73-76 +#ifdef _MSC_VER +// Tell the windows linker to export the type_info symbol required by exceptions +#pragma comment(linker, "/export:??_7type_info@@6B@")

[PATCH] D108905: [ItaniumCXXABI] Make __cxa_end_catch calls unconditionally nounwind

rsmith added a comment. I've forwarded the question here onto CWG, with Arthur's example and the suggestion that maybe we shouldn't allow throwing an exception with a non-noexcept destructor. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D106994: [modules] Fix miscompilation when using two RecordDecl definitions with the same name.

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. LGTM Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:3328-3331 // If there's no definition yet, then DC's definition is added by an update // record, but

[PATCH] D106876: Remove non-affecting module maps from PCM files.

rsmith added a comment. This should have a testcase. I would imagine you can test this by constructing a situation where you build a module twice with different sets of module map files being considered and checking that you get bit-for-bit identical outputs. (For example: set up a temporary

[PATCH] D108742: [WIP] Reclassify form-feed and vertical tab as vertical WS for the purposes of lexing.

rsmith added a comment. In D108742#2970283 , @cor3ntin wrote: >> Drive-by observation: under P2348 , Clang's >> behavior of treating `\n\r` as a single new-line would be "non-standard" >> (requiring special phase 1

[PATCH] D108742: [WIP] Reclassify form-feed and vertical tab as vertical WS for the purposes of lexing.

rsmith added a comment. I assume this is intended to form part of the implementation of https://wg21.link/p2348 and so shouldn't be considered for review right now? Drive-by observation: under P2348 , Clang's behavior of treating `\n\r` as a single new-line

[PATCH] D108680: PR48030: Fix COMDAT-related linking problem with C++ thread_local static data members.

This revision was automatically updated to reflect the committed changes. Closed by commit rGcd4d6d718b2e: PR48030: Fix COMDAT-related linking problem with C++ thread_local static data… (authored by rsmith). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D108680: PR48030: Fix COMDAT-related linking problem with C++ thread_local static data members.

rsmith added a comment. Note that there's a potential ABI impact here: because we accidentally put the `__cxx_global_init` function into a COMDAT with the variable, that meant that the `_ZTH` symbol (which is an alias to `__cxx_global_init`) also pointed into that same COMDAT. I'm not sure how

[PATCH] D67429: Improve code generation for thread_local variables:

rsmith added a comment. https://reviews.llvm.org/D108680 Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67429/new/ https://reviews.llvm.org/D67429 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D108680: PR48030: Fix COMDAT-related linking problem with C++ thread_local static data members.

rsmith created this revision. rsmith added a reviewer: rjmccall. rsmith requested review of this revision. Herald added subscribers: cfe-commits, aheejin. Herald added a project: clang. Previously when emitting a C++ guarded initializer, we tried to work out what the enclosing function would be

[PATCH] D67429: Improve code generation for thread_local variables:

rsmith added a comment. In D67429#2961873 , @rjmccall wrote: > @rsmith, can you fix or revert? Sorry for the long delay here. Looking now. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67429/new/

[PATCH] D104975: Implement P1949

rsmith added inline comments. Comment at: clang/include/clang/Basic/DiagnosticLexKinds.td:117 +def err_character_not_allowed : Error< + "character not allowed in identifiers">; def ext_unicode_whitespace : ExtWarn< The old diagnostic text here was bad in the

[PATCH] D104975: Implement P1949

rsmith added a comment. In D104975#2944313 , @cor3ntin wrote: > @rsmith: I modified the script locally to support dxx: dup P - let me > know if you think that's a good solution Not sure if that's a typo: did you mean "sup" rather than "dup"? In

[PATCH] D104975: Implement P1949

rsmith added inline comments. Comment at: clang/test/CXX/drs/dr2xx.cpp:600 -namespace dr248 { // dr248: yes c++11 - // FIXME: Should this also apply to c++98 mode? This was a DR against C++98. aaron.ballman wrote: > cor3ntin wrote: > > aaron.ballman wrote: >

[PATCH] D107843: [X86] Add parentheses around casts in some of the X86 intrinsic headers.

rsmith added a comment. I found a few more macro hygiene issues in these headers. Comment at: clang/lib/Headers/avx2intrin.h:23 #define _mm256_mpsadbw_epu8(X, Y, M) \ (__m256i)__builtin_ia32_mpsadbw256((__v32qi)(__m256i)(X), \

[PATCH] D106302: Implement P1937 consteval in unevaluated contexts

rsmith added inline comments. Comment at: clang/test/SemaCXX/cxx2a-consteval.cpp:612 +static_assert(is_same::value); + +} // namespace unevaluated cor3ntin wrote: > aaron.ballman wrote: > > cor3ntin wrote: > > > cor3ntin wrote: > > > > aaron.ballman wrote: > > >

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

rsmith added a comment. In D106394#2923186 , @dblaikie wrote: > In D106394#2922972 , @rsmith wrote: > >> ... As an alternative, have you considered checking whether any of the >> headers named in the pragma are

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

rsmith added inline comments. Comment at: clang/lib/Lex/Pragma.cpp:1996 AddPragmaHandler("clang", new PragmaSystemHeaderHandler()); + AddPragmaHandler("clang", new PragmaIncludeInsteadHandler()); AddPragmaHandler("clang", new PragmaDebugHandler());

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

rsmith added a comment. In D106394#2923007 , @cjdb wrote: > In D106394#2922972 , @rsmith wrote: > >> As an alternative, have you considered checking whether any of the headers >> named in the pragma are in the

[PATCH] D107292: [clang] adds warning to alert user when they use alternative tokens as references

rsmith added a comment. We should also warn on: - `and` or `bitand` used as a ref-qualifier in a member function declaration - `xor` used to declare a block pointer type or block pointer literal - `bitand` used as address-of and `and` used to form a GNU address-of-label - `compl` used to name a

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

rsmith added a comment. I'm not in love with the design of this feature. Basing the accept / warn decision on whether the inclusion is in a system header seems fragile and doesn't seem to enforce the intended semantics of the warning (that you can only reach the implementation detail header

[PATCH] D105225: [clang] Add support for optional flag -fnew-infallible to restrict exception propagation

rsmith added a comment. Sorry I'm late to the party here... is there any work ongoing to add this to GCC too? If not, would it make sense to send a quick note to the GCC development list pointing this out to reduce the chance that they add a similar feature with a different flag name?

[PATCH] D107049: [clang-repl] Re-implement clang-interpreter as a test case.

rsmith accepted this revision. rsmith added inline comments. This revision is now accepted and ready to land. Comment at: clang/unittests/Interpreter/InterpreterTest.cpp:64 +// address of main, and some platforms can't implement GetMainExecutable +// without being given the

[PATCH] D106994: [modules] Fix miscompilation when using two RecordDecl definitions with the same name.

rsmith added inline comments. Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:832 + RD->setCompleteDefinition(false); + Reader.mergeDefinitionVisibility(OldDef, RD); +} else { vsapsai wrote: > rsmith wrote: > > vsapsai wrote: > > > Here is

[PATCH] D106994: [modules] Fix miscompilation when using two RecordDecl definitions with the same name.

rsmith added a comment. In D106994#2911903 , @vsapsai wrote: > In D106994#2911508 , @rsmith wrote: > >> For what it's worth, I think the right way to handle this in C is to >> properly implement the "compatible

[PATCH] D106876: Remove non-affecting module maps from PCM files.

rsmith added a comment. I think module maps that specify textual headers should be considered inputs if we entered any of those textual headers; we won't treat those as being imported. In addition to considering the current module and its imports, perhaps we should walk the preprocessor's

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