[PATCH] D114639: Raise the minimum Visual Studio version to VS2019

aaron.ballman added a comment. In D114639#3158401 , @RKSimon wrote: >> Have you checked whether there are any bots in the lab that will need to be >> updated? > > I did find a number of bots that I think need addressing, I am intending to > privately em

[PATCH] D113518: [clang][Sema] Create delegating constructors even in templates

aaron.ballman added inline comments. Comment at: clang/lib/Sema/SemaDeclCXX.cpp:4524-4525 if (!Dependent) { -if (Context.hasSameUnqualifiedType(QualType(ClassDecl->getTypeForDecl(),0), - BaseType)) +if (Delegating) return Bu

[PATCH] D114149: [clang-tidy] Fix pr48613: "llvm-header-guard uses a reserved identifier"

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. In D114149#3144614 , @salman-javed-nz wrote: > I reverted my changes to do with the invalid character substitution. Doing > something a

[PATCH] D114639: Raise the minimum Visual Studio version to VS2019

aaron.ballman added a comment. In D114639#3161303 , @erichkeane wrote: > IMO, if we're updating the MSVC versions, we should do the same for the > GCC/Clang/AppleClang versions too. For example, GCC 5.1 is from 2015, and > Clang 3.5 is from 2014. I'd

[PATCH] D114639: Raise the minimum Visual Studio version to VS2019

aaron.ballman added a comment. In D114639#3161440 , @erichkeane wrote: > In D114639#3161362 , @aaron.ballman > wrote: > >> In D114639#3161303 , @erichkeane >> wrote: >>

[PATCH] D112024: [clang] diagnose_as attribute for Fortify diagnosing like builtins.

aaron.ballman added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:5956 +given. In addition, the order in which arguments should be applied must also +be given. + We should mention that the attribute cannot be applied to a member function (an

[PATCH] D113946: [libc][clang-tidy] fix namespace check for externals

aaron.ballman accepted this revision. aaron.ballman added a comment. LGTM aside from a tiny nit with the documentation. Comment at: libc/docs/clang_tidy_checks.rst:71 +There are exceptions for the following functions: +``__errno_location`` so that errno can be set +``malloc``,

[PATCH] D112626: Convert float to double on __builtin_dump_struct

aaron.ballman added a reviewer: paulsemel. aaron.ballman added a subscriber: paulsemel. aaron.ballman added a comment. Adding @paulsemel to the reviewer list as he was the original author of this functionality (I commit on his behalf which is how I showed up on the git blame). ===

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

aaron.ballman added a comment. There is no information in this patch as to why the changes are needed or useful; please use a more descriptive summary next time. Comment at: clang/unittests/Interpreter/InterpreterTest.cpp:205-207 + // FIXME: We cannot yet handle delayed templ

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

aaron.ballman added inline comments. Comment at: clang/unittests/Interpreter/InterpreterTest.cpp:205-207 + // FIXME: We cannot yet handle delayed template parsing. If we run with + // -fdelayed-template-parsing we try adding the newly created decl to the + // active PTU which

[PATCH] D114562: [clang][docs] Inclusive language: remove use of sanity check in option description

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! Comment at: clang/docs/ThreadSafetyAnalysis.rst:471 - + ``-Wthread-safety-attributes``: Sanity checks on attribute syntax. + + ``-Wthread-safety-attri

[PATCH] D112648: [clang-tidy] Improve fix-its for `modernize-pass-by-value` check

aaron.ballman added a comment. Thanks for your patience! Comment at: clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp:198 +for (const ParmVarDecl *ParmDecl : collectParamDecls(Ctor, ParamDecl)) { + auto ParamTL = ParmDecl->getTypeSourceInfo()->getTypeLoc();

[PATCH] D113828: [clang-tidy] Fix false positives in `fuchsia-trailing-return` check involving deduction guides

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! Can you add a release note about the bug fix? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113828/new/ https://reviews.llvm

[PATCH] D114299: [clang-tidy] Fix `readability-redundant-declaration` false positive for template friend declaration

aaron.ballman added a comment. Hmm, the test case you added passes without the patch applied: https://godbolt.org/z/T9TerMYGz I think the issue is that the class needs to be an instantiated template type: https://godbolt.org/z/bhznPdsvf, and a second (interesting but contrived) test case would

[PATCH] D111400: [Clang] Implement P2242R3

aaron.ballman added a comment. In D111400#3088321 , @aaron.ballman wrote: > In D111400#3088133 , > @hubert.reinterpretcast wrote: > >> In D111400#3087877 , >> @aaron.ba

[PATCH] D113749: [Clang] Fix nesting of discarded and immediate contexts.

aaron.ballman added inline comments. Comment at: clang/include/clang/Sema/Sema.h:1300-1301 +bool InDiscardedStatement : 1; +bool InImmediateFunctionContext : 1; + No real benefit to making these bit-fields, and it'd be good to add some documentation ab

[PATCH] D114688: [Clang] Add __builtin_elementwise_ceil

aaron.ballman added inline comments. Comment at: clang/include/clang/Basic/Builtins.def:649 BUILTIN(__builtin_elementwise_min, "v.", "nct") +BUILTIN(__builtin_elementwise_ceil, "v.", "nct") BUILTIN(__builtin_reduce_max, "v.", "nct") If we're doing ceil, should

[PATCH] D113749: [Clang] Fix nesting of discarded and immediate contexts.

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! Thank you for the fix! Comment at: clang/lib/Sema/SemaExpr.cpp:16575-16578 + ExprEvalContexts[ExprEvalContexts.size() - 2] + .isDiscardedSta

[PATCH] D113749: [Clang] Fix nesting of discarded and immediate contexts.

aaron.ballman closed this revision. aaron.ballman added a comment. I've commit on your behalf in 6eeda06c1d22da2b9fe96a2569a8a0f8e4f36880 , thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://rev

[PATCH] D114439: [Annotation] Allow parameter pack expansions in annotate attribute

aaron.ballman requested changes to this revision. aaron.ballman added inline comments. This revision now requires changes to proceed. Comment at: clang/include/clang/Basic/Attr.td:544-546 + // Set to true for attributes that support parameter pack expansion in its + // argument

[PATCH] D112648: [clang-tidy] Don't offer partial fix-its for `modernize-pass-by-value`

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM aside from a small commenting nit. Comment at: clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp:194 + // If we received a `const&` type, we nee

[PATCH] D113828: [clang-tidy] Fix false positives in `fuchsia-trailing-return` check involving deduction guides

aaron.ballman closed this revision. aaron.ballman added a comment. In D113828#3165101 , @fwolff wrote: > In D113828#3164016 , @aaron.ballman > wrote: > >> LGTM! Can you add a release note about the bug fix? > > D

[PATCH] D114809: Fix documentation for `forEachLambdaCapture` and `hasAnyCapture`

aaron.ballman accepted this revision. aaron.ballman added a comment. LGTM! I had no idea... (As a follow-up question, should we be removing the `using` declarations starting around line 141 or so? Many of them are not used, and if the documentation dumper can't look through typedefs and gets th

[PATCH] D112626: Convert float to double on __builtin_dump_struct

aaron.ballman added inline comments. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2090-2094 +// Variadic functions expect the caller to promote float to double. +if (CanonicalType == Context.FloatTy) { + FieldPtr = + CGF.Builder.CreateFPExt(FieldPtr, CGF.Conv

[PATCH] D114809: Fix documentation for `forEachLambdaCapture` and `hasAnyCapture`

aaron.ballman added a comment. In D114809#3166602 , @ymandel wrote: > In D114809#3165214 , @aaron.ballman > wrote: > >> LGTM! I had no idea... >> >> (As a follow-up question, should we be removing the `using` dec

[PATCH] D114688: [Clang] Add __builtin_elementwise_ceil

aaron.ballman added a comment. In D114688#3166870 , @junaire wrote: > I didn't know if I understand Aaron's words right, but I think I can update > the patch first... I think you understood my suggestion pretty well, thanks! Comment

[PATCH] D114439: [Annotation] Allow parameter pack expansions in annotate attribute

aaron.ballman added inline comments. Comment at: clang/include/clang/Basic/Attr.td:544-546 + // Set to true for attributes that support parameter pack expansion in its + // arguments. + bit ParmExpansionArgsSupport = 0; steffenlarsen wrote: > aaron.ballman wro

[PATCH] D108560: [clang-tidy] Add support for `NOLINTBEGIN` ... `NOLINTEND` comments

aaron.ballman added a comment. In D108560#3167621 , @ymandel wrote: > Aaron, Salman, > > We've seen a serious perfomance regression caused by this patch. The issue is > that the new code scans every file in its entirety for every single > diagnostic enc

[PATCH] D108560: [clang-tidy] Add support for `NOLINTBEGIN` ... `NOLINTEND` comments

aaron.ballman added a comment. In D108560#3167830 , @carlosgalvezp wrote: > Good catch! That explains the performance drop we observed as well. > > We are however currently relying on this feature so it would be sad to revert > it. Would it be possible/

[PATCH] D108560: [clang-tidy] Add support for `NOLINTBEGIN` ... `NOLINTEND` comments

aaron.ballman added a comment. In D108560#3167847 , @salman-javed-nz wrote: > Thanks for the investigation. Just exploring the options... > > With regards to reverting it, is the cat out of the bag? I already see some > usage of the NOLINTBEGIN feature

[PATCH] D108560: [clang-tidy] Add support for `NOLINTBEGIN` ... `NOLINTEND` comments

aaron.ballman added a comment. In D108560#3167912 , @salman-javed-nz wrote: > In D108560#3167883 , @aaron.ballman > wrote: > >> In D108560#3167847 , >> @salman-javed-nz

[PATCH] D114602: [clang-tidy][docs][NFC] Improve documentation of bugprone-unhandled-exception-at-new

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM aside from a small nit. Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-unhandled-exception-at-new.rst:16-17 + +The exception handler is che

[PATCH] D114292: [clang-tidy] Fix `altera-struct-pack-align` check for empty structs

aaron.ballman added a comment. +1 to the request for a release note, but otherwise this LGTM (with or without the `static_cast` changes) in general. Should we also do something special for `[[no_unique_address]]` (if we should, I'm fine doing that in a follow-up)? Repository: rG LLVM Github

[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

aaron.ballman added a comment. In D108643#3158232 , @aaron.ballman wrote: > In D108643#3140927 , @aaron.ballman > wrote: > >> In D108643#3093323 , >> @aaron.ballman wro

[PATCH] D114080: [SYCL] Diagnose uses of zero length arrays

aaron.ballman added a comment. Just a few minor nits from me. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11395-11396 InGroup; +def err_sycl_zero_array_size : Error< + "zero-length arrays are not permitted in SYCL device code">; We should

[PATCH] D136018: [Clang] Fix crash when checking misaligned member with dependent type

aaron.ballman accepted this revision. aaron.ballman 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/D136018/new/ https://reviews.llvm.org/D136018

[PATCH] D135920: [clang][Sema] Use correct array size for diagnostic

aaron.ballman added inline comments. Comment at: clang/test/SemaCXX/array-bounds.cpp:240 -((char*)foo)[sizeof(foo)] = '\0'; // expected-warning {{array index 32768 is past the end of the array (which contains 32768 elements)}} +((char*)foo)[sizeof(foo)] = '\0'; // ex

[PATCH] D135025: [clang][Interp] Support base class constructors

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. Okay, this is incremental progress, so LGTM! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135025/new/ https://reviews.llvm.org/D135025

[PATCH] D136013: [clang][Interp] Fix ignoring expression return values

aaron.ballman added inline comments. Comment at: clang/test/AST/Interp/records.cpp:208 + +struct S { + int a = 0; Hmmm, this feels related to the discarded value results changes, but it might be a test case for a different scenario as well, so take this or leav

[PATCH] D135513: [clang][Interp] Check instance pointers before calling functions on them

aaron.ballman added inline comments. Comment at: clang/lib/AST/Interp/Function.cpp:39 + assert(It != SrcMap.end()); + It--; // We want the offset *before* the given one. return It->second; tbaeder wrote: > While I think the comment here is correct, the decre

[PATCH] D136036: [Clang] Add __has_constexpr_builtin support

aaron.ballman added a comment. In D136036#3863519 , @Izaron wrote: >> It looks like unrelated formatting changes snuck in to this file. > > @aaron.ballman JFYI after I reverted what `git clang-format HEAD~1` did to > the code, the build has failed > >

[PATCH] D136036: [Clang] Add __has_constexpr_builtin support

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM, but please wait for @shafik before landing, in case he has more feedback or questions. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.l

[PATCH] D136160: [Attr][Doc] Fix pragma unroll documentation.

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM, thank you for the fix! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136160/new/ https://reviews.llvm.org/D136160 _

[PATCH] D136022: [clang] Add time profile for constant evaluation

aaron.ballman added a comment. Thank you for working on this, having more detailed timing information for where we're spending time in the frontend will really help us to evaluate where we need to work on frontend performance. Comment at: clang/lib/AST/ExprConstant.cpp:15257

[PATCH] D135557: Add needsImplicitDefaultConstructor and friends

aaron.ballman added inline comments. Comment at: clang/bindings/python/clang/cindex.py:1530 + +def record_needs_implicit_default_constructor(self): +"""Returns True if the cursor refers to a C++ record declaration dblaikie wrote: > anderslanglands wro

[PATCH] D133668: [HLSL] Disable integer promotion to avoid int16_t being promoted to int for HLSL.

aaron.ballman added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:841 +if (Ty->isPromotableIntegerType() && +// Avoid promote integer type to int. +!getLangOpts().HLSL) { rjmccall wrote: > Hmm, the downside to the change being appl

[PATCH] D135920: [clang][Sema] Use correct array size for diagnostic

aaron.ballman accepted this revision. aaron.ballman added a comment. LGTM, I think the new diagnostic wording is an improvement. Can you please add a release note to mention the diagnostic wording was patched up? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews

[PATCH] D135557: Add needsImplicitDefaultConstructor and friends

aaron.ballman added inline comments. Comment at: clang/bindings/python/clang/cindex.py:1530 + +def record_needs_implicit_default_constructor(self): +"""Returns True if the cursor refers to a C++ record declaration dblaikie wrote: > anderslanglands wro

[PATCH] D135513: [clang][Interp] Check instance pointers before calling functions on them

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

[PATCH] D135060: [HLSL] Add groupshare address space.

aaron.ballman added inline comments. Comment at: clang/lib/Sema/SemaDecl.cpp:10083 } +// HLSL not support address space on a function return type declaration. +LangAS AddressSpace = NewFD->getReturnType().getAddressSpace(); Commen

[PATCH] D134453: Disambiguate type names when printing NTTP types

aaron.ballman added a comment. In D134453#3868821 , @DoDoENT wrote: > In D134453#3868789 , @dblaikie > wrote: > >> I still don't think "keep full NTTP type printing behind a policy, for those >> that want/need t

[PATCH] D135060: [HLSL] Add groupshare address space.

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM Comment at: clang/test/ParserHLSL/group_shared_202x.hlsl:5-6 + +// expected-error@+1 {{return type cannot be qualified with address space}} +auto l = []()

[PATCH] D134453: Disambiguate type names when printing NTTP types

aaron.ballman added a comment. In D134453#3869610 , @dblaikie wrote: > In D134453#3869201 , @aaron.ballman > wrote: > >> In D134453#3868821 , @DoDoENT >> wrote: >> >>> I

[PATCH] D134878: Update developer policy on potentially breaking changes

aaron.ballman added a comment. In D134878#3869947 , @mehdi_amini wrote: > From what I saw when you posted the discourse thread initially, I understand > you're targeting user-visible features, and mostly from the "toolchain" side > of the project. > Ho

[PATCH] D135764: [clang][Interp] Implement for loops

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM aside from some minor points. Comment at: clang/lib/AST/Interp/ByteCodeStmtGen.cpp:361 + this->emitLabel(IncLabel); + if (Inc && !this->discard(Inc)) +

[PATCH] D136343: [Lex] Add compatibility with MSVC

aaron.ballman added a reviewer: aaron.ballman. aaron.ballman added a comment. Thank you for the patch! It looks like it's missing test coverage for the changes, can you add some to `clang/test/Preprocessor`? One question I have is: if we're going to support `__FUNCTION__`, shouldn't we also sup

[PATCH] D135013: [clang][Interp] Array initialization via ImplicitValueInitExpr

aaron.ballman added a comment. In D135013#3870894 , @tbaeder wrote: > @aaron.ballman Do you maybe have a different reproducer or know more about > the expected behavior of a `ImplicitValueInitExpr` for array and record types? So here's another reproduce

[PATCH] D135557: Add needsImplicitDefaultConstructor and friends

aaron.ballman added inline comments. Comment at: clang/bindings/python/clang/cindex.py:1530 + +def record_needs_implicit_default_constructor(self): +"""Returns True if the cursor refers to a C++ record declaration anderslanglands wrote: > dblaikie wro

[PATCH] D135013: [clang][Interp] Array initialization via ImplicitValueInitExpr

aaron.ballman added a comment. In D135013#3871250 , @tbaeder wrote: > In D135013#3871209 , @aaron.ballman > wrote: > >> In D135013#3870894 , @tbaeder >> wrote: >> >>> @a

[PATCH] D134453: Disambiguate type names when printing NTTP types

aaron.ballman added a comment. In D134453#3871251 , @dblaikie wrote: > @aaron.ballman: thanks for tracking down the source of confusion between our > perspectives. (& yeah, sorry, could've included less ambiguous examples with > the extra nesting so we'

[PATCH] D134453: Disambiguate type names when printing NTTP types

aaron.ballman added a comment. In D134453#3871386 , @dblaikie wrote: >> In terms of next steps, I think we should try to see if there's a "clear" >> path forward in terms of printing the types vs not printing the types. If we >> find one, then great, we

[PATCH] D133668: [HLSL] Disable integer promotion to avoid int16_t being promoted to int for HLSL.

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! Please give @rjmccall a few days to look it over as well before landing. Comment at: clang/include/clang/AST/ASTContext.h:2379 + /// More type predicat

[PATCH] D136013: [clang][Interp] Fix ignoring expression return values

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136013/new/ https://reviews.llvm.org/D136013 ___ cfe-commits mailing lis

[PATCH] D135013: [clang][Interp] Array initialization via ImplicitValueInitExpr

aaron.ballman added inline comments. Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:726 + } else if (const auto *IVIE = dyn_cast(Initializer)) { +auto ArrayType = IVIE->getType()->getAsArrayTypeUnsafe(); +assert(ArrayType); Please spell out the typ

[PATCH] D135557: Add needsImplicitDefaultConstructor and friends

aaron.ballman added inline comments. Comment at: clang/bindings/python/clang/cindex.py:1530 + +def record_needs_implicit_default_constructor(self): +"""Returns True if the cursor refers to a C++ record declaration dblaikie wrote: > aaron.ballman wrote

[PATCH] D135557: Add needsImplicitDefaultConstructor and friends

aaron.ballman added a comment. In D135557#3871482 , @dblaikie wrote: > I was hoping the rephrasing (is this really a question about which ctors the > type has, or about how the type can be constructible) might offer us a way > out for this use case, at

[PATCH] D135557: Add needsImplicitDefaultConstructor and friends

aaron.ballman added a comment. In D135557#3871716 , @dblaikie wrote: > (I'm still sort of curious how the AST matchers deal with all this - I guess > they must have Sema available, because I'd assume they make all sorts of > queries like "is this constr

[PATCH] D136355: [clang][Sema] Fix caret position to be on the non null parameter

aaron.ballman added a comment. Thank you for the fix! Can you add test coverage to show the behavioral change? You can either position the problematic null argument on its own line (so you can validate the correct position) or use `-fdiagnostics-print-source-range-info` to check a specific ran

[PATCH] D136022: [clang] Add time profile for constant evaluation

aaron.ballman added inline comments. Comment at: clang/unittests/Support/TimeProfilerTest.cpp:197-198 + + // NOTE: If this test is failing, run this test with + // `llvm::errs() << TraceGraph;` and change the assert above. +} This bit worries me because I suspe

[PATCH] D135013: [clang][Interp] Array initialization via ImplicitValueInitExpr

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. Aside from a naming quirk, LGTM Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:726 + } else if (const auto *IVIE = dyn_cast(Initializer)) { +const Ar

[PATCH] D136424: Update links to googletest documentation

aaron.ballman added a comment. In D136424#3874180 , @DerKasper wrote: > I am not sure, what i have to do now, after this was accepted? > In the documentation i find: > "If you do not have commit access, please let people know during the review > and so

[PATCH] D136424: Update links to googletest documentation

aaron.ballman added a comment. In D136424#3874196 , @sylvestre.ledru wrote: > oh, crappy, i forgot to update the author of the patch :( > really sorry about that... :( No worries, it happens (I've made the same mistake a few times as well)! If @DerKas

[PATCH] D136416: [AST] Support Bool type in va_arg

aaron.ballman added a comment. > Currently, if the second argument is "Bool", it will be casted to "int". That behavior is required by the standard, which is why we give a warning about it being undefined behavior due to the promotion. For C, see C2x 7.16.1.1p2 ("If //type// is not compatible w

[PATCH] D136343: [Lex] Add compatibility with MSVC

aaron.ballman added a comment. In D136343#3873284 , @Qfrost911 wrote: > I understand you mean, but I can't get function name at macro expedition > stage. Correct! We have a `PredefinedExpr` AST node type (https://github.com/llvm/llvm-project/blob/main/

[PATCH] D136416: [AST] Support Bool type in va_arg

aaron.ballman added a comment. In D136416#3874308 , @Qfrost911 wrote: > I see, thank you very much. Thank you for trying to improve things in Clang! Even if this wasn't the right approach, the effort is still appreciated. :-) BTW, we have been trying to

[PATCH] D136449: [Clang] Implement P2513

aaron.ballman added a comment. Thanks for this! Can you add more details to the patch summary as to what this paper does? Paper numbers help, but don't convey a whole lot of information at a glance if we need to come back to this review for code archeology. Please also add a test to the correct

[PATCH] D136451: GH58368: Correct concept checking in a lambda defined in concept

aaron.ballman added a comment. Generally, this looks reasonable to me. I did spot a few things, but nothing major. Should this change come with a release note for the fix? Comment at: clang/include/clang/AST/DeclTemplate.h:3310 +// during constraint checking. +class ConceptSpe

[PATCH] D136449: [Clang] Implement P2513

aaron.ballman added a comment. In D136449#3875159 , @cor3ntin wrote: > In D136449#3874867 , @aaron.ballman > wrote: > >> Thanks for this! Can you add more details to the patch summary as to what >> this paper do

[PATCH] D136355: [clang][Sema] Fix caret position to be on the non null parameter

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM aside from a tiny grammar nit. Thank you for the fix! If you'd like me to commit this on your behalf, I can fix up the release note myself when landing, but please let me kn

[PATCH] D136022: [clang] Add time profile for constant evaluation

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! Please be sure to add a release note to alert folks to the time profiler becoming more awesome. :-) Comment at: clang/unittests/Support/TimeProfilerTest.

[PATCH] D134453: Disambiguate type names when printing NTTP types

aaron.ballman added a comment. In D134453#3872355 , @dblaikie wrote: > But yeah, not sure/open to perspectives. > > @aaron.ballman - member names V type names V both? I think type names are really the only thing that will disambiguate the expressions in

[PATCH] D136449: [Clang] Implement P2513

aaron.ballman added inline comments. Comment at: clang/lib/Frontend/InitPreprocessor.cpp:701 +Builder.defineMacro("__cpp_char8_t", +LangOpts.CPlusPlus20 ? "202207L" : "201811L"); Builder.defineMacro("__cpp_impl_destroying_delete", "201806L"); --

[PATCH] D136355: [clang][Sema] Fix caret position to be on the non null parameter

This revision was automatically updated to reflect the committed changes. Closed by commit rG1534b048d6fd: Fix caret position to be on the non null parameter (authored by Grillo, committed by aaron.ballman). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.o

[PATCH] D136449: [Clang] Implement P2513

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM aside from a missing full stop in a comment. Tom's on vacation most of this week, so if he has any concerns, they can be address post-commit unless you want to wait for him

[PATCH] D136451: GH58368: Correct concept checking in a lambda defined in concept

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. It looks like precommit CI caught a failing test case that should be fixed before landing, but otherwise this LGTM. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136451/

[PATCH] D136528: [clang][Interp] Implement add and sub compound assign operators

aaron.ballman added inline comments. Comment at: clang/test/AST/Interp/literals.cpp:412 + + constexpr int getTwo() { +int i = 1; I'd also like some test cases where the result of the operation is discarded. e.g., ``` constexpr int func() { int i = 12; i

[PATCH] D136423: [clang][Interp] Implement inc/dec postfix and prefix operators

aaron.ballman added inline comments. Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:1237-1240 +if (DiscardResult) + return this->emitIncPop(*T, E); + +return this->emitInc(*T, E); Style question: should we prefer doing something like: `return D

[PATCH] D136528: [clang][Interp] Implement add and sub compound assign operators

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136528/new/ https://reviews.llvm.org/D136528 ___ cfe-commits mailing lis

[PATCH] D135557: Add needsImplicitDefaultConstructor and friends

aaron.ballman added a comment. In D135557#3872324 , @dblaikie wrote: > In D135557#3871803 , @aaron.ballman > wrote: > >> In D135557#3871716 , @dblaikie >> wrote: >> >>>

[PATCH] D136532: [clang][Interp] Implement left and right shifts

aaron.ballman added inline comments. Comment at: clang/lib/AST/Interp/Interp.h:1236-1237 inline bool ShiftRight(InterpState &S, CodePtr OpPC, const T &V, unsigned RHS) { if (RHS >= V.bitWidth()) { S.Stk.push(T::from(0, V.bitWidth())); } else { This d

[PATCH] D136423: [clang][Interp] Implement inc/dec postfix and prefix operators

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM aside from nits. Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:1275-1277 +if (DiscardResult) + return this->emitPop(*T, E); return thi

[PATCH] D136156: [Clang][Diagnostic] Add hidden-reinterpret-cast diagnostic warning

aaron.ballman added a comment. Thank you for the patch! We don't typically add new diagnostics that are ignored by default because we have significant evidence that users don't enable them enough to warrant adding them. In this case, I don't think we should enable this by default because it's

[PATCH] D136604: Add clang_CXXMethod_isCopyAssignmentOperator to libclang

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! Would you like to obtain commit access to land this one on your own (https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access)? If not, that's fine, I can also l

[PATCH] D127462: [Clang] Begin implementing Plan 9 C extensions

aaron.ballman added a reviewer: aaron.ballman. aaron.ballman added a comment. In D127462#3877815 , @ksaunders wrote: > Ping. Sorry for not noticing this patch was languishing! As far as the patch itself goes, I don't see any major concerns. Because thi

[PATCH] D136604: Add clang_CXXMethod_isCopyAssignmentOperator to libclang

aaron.ballman added a comment. In D136604#3881949 , @diseraluca wrote: > In D136604#3879928 , @aaron.ballman > wrote: > >> LGTM! >> >> Would you like to obtain commit access to land this one on your own >> (htt

[PATCH] D136532: [clang][Interp] Implement left and right shifts

aaron.ballman added inline comments. Comment at: clang/test/AST/Interp/shifts.cpp:57 +//c >>= 99; // expected-warning {{shift count >= width of type}} +//c <<= CHAR_BIT; // expected-warning {{shift count >= width of type}} +//c >>= CHAR_BIT; // expected-warning {{

[PATCH] D136670: [clang][Interp] Fix InterpFrame::describe() for This pointers

aaron.ballman accepted this revision. aaron.ballman 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/D136670/new/ https://reviews.llvm.org/D136670

[PATCH] D134902: [clang] Implement -fstrict-flex-arrays=3

aaron.ballman added a comment. In D134902#3848616 , @kees wrote: > In D134902#3848595 , > @serge-sans-paille wrote: > >> I second the opinion here. C99 says nothing about flexible array member for >> unions, tha

[PATCH] D134902: [clang] Implement -fstrict-flex-arrays=3

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. In D134902#3883209 , @void wrote: > In D134902#3882255 , @aaron.ballman > wrote: > >> One thin

[PATCH] D136120: [Clang] follow-up D128745, remove ClangABICompat checks

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM, thank you! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136120/new/ https://reviews.llvm.org/D136120 _

<    47   48   49   50   51   52   53   54   55   56   >